Support from Acquia helps fund testing for Drupal Acquia logo

Comments

jamsilver’s picture

Assigned: jamsilver » Unassigned
Status: Needs work » Needs review
FileSize
1.02 KB

patch attached

Status: Needs review » Needs work

The last submitted patch, services-1693342-01.patch, failed testing.

ygerasimov’s picture

Priority: Normal » Critical

oh! this is really nasty bug. lets have test to make sure it will never happen again.

mradcliffe’s picture

Added a test class to test a disabled resource (I chose the system resource). Is this the approach you want to take for the test, ygerasimov?

  1. Patch from jamsilver.
  2. Patch with test.
  3. Combined patch.

Status: Needs review » Needs work

The last submitted patch, services-1693342-04.patch, failed testing.

mradcliffe’s picture

Status: Needs work » Needs review

For some reason the commit I made (2nd patch) didn't include changes to files, just the added test, so it didn't get run. The last patch demonstrates it completely though (the git apply one). Sorry.

kylebrowning’s picture

Ugh, why are tests failing, we need to fix those issues if they are failing in their current state. Ill take a look at this tomorrow.

mradcliffe’s picture

#4: services-1693342-04.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, services-1693342-04.patch, failed testing.

mradcliffe’s picture

Looking into the other tests.

mradcliffe’s picture

Status: Needs work » Needs review
FileSize
6.14 KB

saveNewEndpoint doesn't add the target_actions array to the endpoint, which was introduced in #1484992: Attach file to node targeted_action. There were tests added in that patch, which makes things fail. After I fixed that, the tests still fail because on my system tests are changin node ids? Furthermore it looks like that issue has been reset to 7.x-3.x branch for further development

Here's a patch with #1694438: Aliased resources are inaccessible applied as well just to see the changing test failures.

Status: Needs review » Needs work

The last submitted patch, 1693342-allow-access-disabled-resources-with-1694438.patch, failed testing.

mradcliffe’s picture

Oh, nice. It was just my test system. Down to the same 6 failures that are in other issues now. :-)

mradcliffe’s picture

Status: Needs work » Needs review
FileSize
2.99 KB

So both user resource and xml rpc tests pass locally for me. I'm going to set this back to needs review for now.

Here are the formatted patches that I applied (note the one from #1694438: Aliased resources are inaccessible).

kylebrowning’s picture

Status: Needs review » Needs work

Yeah unfortunately the tests dont pass at all with these patches.

mradcliffe’s picture

Status: Needs work » Needs review

Edit: Which tests? The same 6 tests that fail on every single issue?

#11: 1693342-allow-access-disabled-resources-with-1694438.patch queued for re-testing.

kylebrowning’s picture

Status: Needs review » Needs work

Nah its way more than 6, Comment tests fails, file upload tests fail and it actually has a 500 error.

kylebrowning’s picture

Patch #11 might be good, but the patches #14 do not.

mradcliffe’s picture

Status: Needs work » Needs review

Hmm, weird. I'll have to look at my local repo again.

mradcliffe’s picture

Status: Needs review » Needs work

Accidentally changed status.

mradcliffe’s picture

I cloned a clean services 7.x-3.x branch, took the combined patch in #11 and git apply'd it. Then checkout a clean services 7.x-3.x branch, applied the patches in #14 and git am'd them. Then git diff'd the two branches, and there were no differences. I'm confused now. I think there might be some issue with the way the patches in #14 were applied. Not sure what happend @kylebrowning.

kylebrowning’s picture

ok ill do the same.

mradcliffe’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 1693342-allow-access-disabled-resources-with-1694438.patch, failed testing.

mradcliffe’s picture

Status: Needs work » Needs review
FileSize
5.48 KB

Pulled out #1694438: Aliased resources are inaccessible from the patch. Setting to review (with do-not-test) until that issue is committed.

kylebrowning’s picture

kylebrowning’s picture

I fixed the tests, lets see what happens!

kylebrowning’s picture

Status: Needs review » Fixed

Well look at that! Committed and fixed in dev!

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.