Problem/Motivation

A contributor submitted a patch generated by diff. The testbot failed to apply the patch, but still ran the tests and reported they passed.

[02:01:19] Invoking operation [apply]...
[02:01:19] Command [git apply --check -p1 /var/lib/drupaltestbot/sites/default/files/review/migrate_simple_xml_empty_issue_2330227_1.patch 2>&1] succeeded.
[02:01:19] Command [git apply -v -p1 /var/lib/drupaltestbot/sites/default/files/review/migrate_simple_xml_empty_issue_2330227_1.patch 2>&1] failed
  Duration 0 seconds
  Directory [/var/lib/drupaltestbot/sites/default/files/checkout/sites/default/modules/migrate]
  Status [1]
 Output: [error: No changes].
[02:01:19] Applied [migrate_simple_xml_empty_issue_2330227_1.patch] to:
 > no files

Proposed resolution

  • Modify DrupalCI\Tests\Application\PatchFailTest so it fails if the testbot reports success despite a failed patch apply.
  • Fix the testbot to fail a patch error.

Remaining tasks

Follow-up with an issue to write a test to make sure the entire runner responds correctly to Patch fail.

User interface changes

API changes

Data model changes

Comments

isntall’s picture

isntall CreditAttribution: isntall at Drupal Association commented
Project: Drupal.org Testbots » DrupalCI: Dispatcher (Modernizing Testbot Initiative)
jthorson’s picture

jthorson CreditAttribution: jthorson commented
Title: Invalid patch file passes testbot » Fail test if patch does not apply successfully.
Project: DrupalCI: Dispatcher (Modernizing Testbot Initiative) » DrupalCI: Test Runner

Flaw in the PIFR test detection code ... not really a DrupalCI issue.

Test Runners will need to determine whether or not a patch applied successfully or not.

cilefen’s picture

cilefen CreditAttribution: cilefen commented
Status: Active » Needs review

@jthorson run.sh dies if the patch does not apply. Is there more needed?

jthorson’s picture

jthorson CreditAttribution: jthorson commented

Yes ... run.sh becomes deprecated with the new test runner build.

So for this, we need to verify that i) /src/DrupalCI/Plugin/BuildSteps/setup/Patch.php returns an error if the patch apply is not successful, and ii) the test execution detects this failure and does not proceed with the rest of the test.

jthorson’s picture

jthorson CreditAttribution: jthorson commented

Incidently, regarding my previous comment, I think we just need to verify this.

This report itself was carried over from the PIFR queue, so this may not actually be an issue on the DrupalCI test runner. We just want to confirm 100% before closing it.

Mile23’s picture

Mile23
Primary language English
Location Seattle, WA
CreditAttribution: Mile23 as a volunteer commented
Title: Fail test if patch does not apply successfully. » Write a test to ensure that DrupalCI\Job\CodeBase\Patch fails the test when the patch does not apply
Issue summary: View changes
Status: Needs review » Needs work

Updated summary based on #4.

Mile23’s picture

Mile23
Primary language English
Location Seattle, WA
CreditAttribution: Mile23 as a volunteer commented
Parent issue: » #2680565: [meta] Create functional tests of run command output to test production essentials
Mile23’s picture

Mile23
Primary language English
Location Seattle, WA
CreditAttribution: Mile23 as a volunteer commented

A test was added in #2683013: Better functional test framework Here: http://cgit.drupalcode.org/drupalci_testbot/tree/tests/DrupalCI/Tests/Ap...

It turns out the test fails, and is being fixed here: #2682129: Failed patch attempt bails horribly for local testing

Mixologic’s picture

Mixologic
He/Him/His
Primary language English
Location Portland, OR
CreditAttribution: Mixologic at Drupal Association commented
Component: Miscellaneous » Testrunner Tests
Mixologic’s picture

Mixologic
He/Him/His
Primary language English
Location Portland, OR
CreditAttribution: Mixologic at Drupal Association commented
Component: Testrunner Tests » Codebase Build
Mile23’s picture

Mile23
Primary language English
Location Seattle, WA
CreditAttribution: Mile23 as a volunteer commented
Title: Write a test to ensure that DrupalCI\Job\CodeBase\Patch fails the test when the patch does not apply » Fail test if patch does not apply successfully
Issue summary: View changes

Poking around in this a bit, the problem wasn't solved in #2682129: Failed patch attempt bails horribly for local testing, so I'm changing the title back.

Adding a branch for this, amending the test so it fails DrupalCI\Tests\Application\PatchFailTest.

  • Mile23 committed f92410a on 2340939-fail-patch-error 
    Issue #2340939 by mikeryan: Fail test if patch does not apply...

  • Mile23 committed 4f68b4f on 2340939-fail-patch-error 
    Issue #2340939 by Mile23: Fail test if patch does not apply successfully
Mile23’s picture

Mile23
Primary language English
Location Seattle, WA
CreditAttribution: Mile23 as a volunteer commented
Status: Needs work » Needs review

That last commit fixes the problem by adding a new behavior: Any code can throw a BuildException subclass (BuildFailException, BuildErrorException), and if that exception bubbles up to RunCommand it will turn into either exit code 1 or 2.

  • Mile23 committed 8164492 on 2340939-fail-patch-error 
    Issue #2340939: JobException subclasses now set their own exit code.

  • Mixologic committed a494767 on 2340939-fail-patch-error 
    Issue #2340939: Rerolls and updates BuildException and handles...
Mixologic’s picture

Mixologic
He/Him/His
Primary language English
Location Portland, OR
CreditAttribution: Mixologic at Drupal Association commented

This adds BuildTaskExceptions, catches them properly, fixes the tests, and additionally adds signal processing to the build process.

  • Mile23 committed 9dbba94 on 2340939-fail-patch-error 
    Issue #2340939: Cleanup on PatchFailTest
Mile23’s picture

Mile23
Primary language English
Location Seattle, WA
CreditAttribution: Mile23 as a volunteer commented
Status: Needs review » Reviewed & tested by the community

Minor fixes to PatchFailTest. Looks good.

Mixologic’s picture

Mixologic
He/Him/His
Primary language English
Location Portland, OR
CreditAttribution: Mixologic at Drupal Association commented
Status: Reviewed & tested by the community » Fixed

Pushed on in. Yay to not having any more skipped tests.

Mixologic’s picture

Mixologic
He/Him/His
Primary language English
Location Portland, OR
CreditAttribution: Mixologic at Drupal Association commented

Status: Fixed » Closed (fixed)

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