Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
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.
Comments
Comment #1
isntall CreditAttribution: isntall at Drupal Association commentedComment #2
jthorson CreditAttribution: jthorson commentedFlaw 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.
Comment #3
cilefen CreditAttribution: cilefen commented@jthorson
run.sh
dies if the patch does not apply. Is there more needed?Comment #4
jthorson CreditAttribution: jthorson commentedYes ... 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.
Comment #5
jthorson CreditAttribution: jthorson commentedIncidently, 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.
Comment #6
Mile23Updated summary based on #4.
Comment #7
Mile23Comment #8
Mile23A 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
Comment #9
MixologicComment #10
MixologicComment #11
Mile23Poking 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
.Comment #14
Mile23That 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 toRunCommand
it will turn into either exit code 1 or 2.Comment #17
MixologicThis adds BuildTaskExceptions, catches them properly, fixes the tests, and additionally adds signal processing to the build process.
Comment #19
Mile23Minor fixes to
PatchFailTest
. Looks good.Comment #20
MixologicPushed on in. Yay to not having any more skipped tests.
Comment #21
Mixologic