Issue:

The initial integration of DrupalCI with drupal.org was built for the "Simpletest Testing" use case, and contains a hardcoded assumption that the build result is captured and passed to drupal.org inside an xml file as output by run-tests.sh.

This leads to challenges in reporting back status in cases where run-tests.sh does not execute ... for example, if a patch does not apply, the current implementation needs to create a fake run-tests.sh xml output file with a placeholder message in order to communicate this status back to drupal.org.

It also adds significant complexity to other potential build types, the most basic of which really only need to report back a simple pass/fail/error status and human readable message.

Proposal:

1. Each BuildTask plugin is responsible for defining its own build artifacts.
2. Any BuildTask plugin can 'abort the build' - this might be via throwing an exception, or returning FALSE when the build is executing.
3. Each BuildTask plugin shall implement a summary string that is short enough for drupal.org to display, that the Build can probe for during execution or abortion.
4. The build shall output a file into the build workspace (where jenkins can see it as an artifact), which contains build metadata. There should be an 'overall status' field that contains whatever the reason was for an aborted build, in addition to the build status itself. This file should be a json file that d.o can pull in and use for reporting back to issues.

CommentFileSizeAuthor
#8 2655718.patch48.32 KBjthorson

Comments

jthorson created an issue. See original summary.

jthorson’s picture

jthorson commented

Work progressing in 2655718-complete-JobResults-implementation feature branch.

mile23’s picture

mile23
Primary language English
Location Seattle, WA
commented

Branch-based dev only works if everyone has push access. :-)

Also, I'm tempted to mark this postponed on #2544340: Make the tests pass which would then require a rebase for 2655718-complete-JobResults-implementation

jthorson’s picture

jthorson commented

Great point on the branch-based development ... this is how the DA asked me to proceed while in Barcelona, but obviously this does not lend itself to community contribution!!!

On the tests front, this leads to a bit of a catch-22 ... We should definitely make the existing tests pass, but I think that adding additional test coverage based on the existing code base runs the risk of a lot of throw-away effort - my preference would be to build out the test coverage after (or at least, in parallel to) completing/refactoring the test runner code, and then go back to fill in the testing gaps. At the very least, the final architecture hasn't been fully determined and documented; and it would be difficult to write tests to an unknown set of requirements.

mile23’s picture

mile23
Primary language English
Location Seattle, WA
commented

Great point on the branch-based development

Well, however you want to do it, this is me asking for the ability to push to that branch.

At the very least, the final architecture hasn't been fully determined and documented; and it would be difficult to write tests to an unknown set of requirements.

That's why I keep pestering you for docs. :-)

What happens is that the tests become the standard over time if there's no spec.

Also, unit tests exist for the developer, so they can see what breaks when they change something. This is important, and why we have to fix the existing tests before going much further at all.

Seriously, it's an 8k patch. #2544340: Make the tests pass

jthorson’s picture

jthorson commented

I'm not arguing against that patch ... I'm speaking of the extension to that issue, which is full test coverage of the test runner.

  • jthorson committed 32d7a17 on 2655718-complete-JobResults-implementation 
    [#2655718] by jthorson: Introduce postprocess step outside of the main...
jthorson’s picture

jthorson commented
StatusFileSize
new 2655718.patch48.32 KB

Here's a start on #1 through #3 ... There's some file renames that blow this up much bigger than it is, so for review purposes, I suggest a commit-by-commit review of the feature branch instead.

Also, this enables --verbose mode on simpletest by default (in order to demonstrate the gathering of build artifacts and showing how we can provide the verbose mode output for troubleshooting simpletest tests), which we probably would NOT want to do by default on the production bots.

jthorson’s picture

jthorson commented
Status: Active » Needs review
jthorson’s picture

jthorson commented

I've added some new commits to the JobResults branch, which take some major steps forward towards better results tracking and handling within the test runner.

The most significant development is an architectural tweak that I believe will ultimately simplify and clean up the test runner codebase. (These benefits aren't realized with these commits, as so far I've only added code so as to maintain backwards compatibility with dev/production; but once the duplication is removed, I believe the resulting code will be easier to follow and understand.)

For context, the vocabulary I'm using below assumes jobs contain one or more build stages, and each build stage contains one or more build steps. For example, 'Setup' is a build stage, with 'Checkout' and 'Patch' being build steps within 'Setup'.

Originally, build step plugins extended the abstract PluginBase class directly. This change introduces an intermediary abstract class (BuildStepBase), which adds three properties: State, Result, and Summary:

  • State refers to the build step execution state (i.e. Waiting/Initalizing/Executing/Completed/Error).
  • Result refers to the build step execution result (i.e. Pending/Pass/Fail/XFail/XPass/Skipped/Warning/Error/SystemError).
  • Summary refers to a human readable string describing the build step execution result.

I'm also looking at adding "Output" to this property list, so that the plugin instance can store it's own console output as part of the result.

This change allows build step plugins to set these properties directly during execution of the build step logic. For example, if the Patch build step plugin encounters a patch which doesn't apply, it can directly update it's own State and Result to "Error" and Summary to "Patch xyz.patch does not apply".

Within the build step execution loop in RunCommand, as soon as a build step plugin has run, the state/result/summary properties for that build step are copied up into the JobResults object so that they can be referenced during post-processing (where they can be referenced using $job->getJobResults()->stages[$stage]['steps'][$step]['state']).

Upon completion of all build steps within a given build stage, if any build step has state other than "Completed", then this results in an "Error" state for the build stage itself. If any build stage has state other than "Completed" at the end of the job, this results in an "Error" state for the entire job.

Note the distinction between "State" and "Result" ... State does not reflect anything about the Pass/Fail status of the test run; it speaks only to the "health" of the test run as a whole. The Result property speaks to the actual 'outcome' of a given build step, stage, or overall job. This gives us a much-needed differentiation between infra/environmental failures and test failures. As an example, only a job with state "Completed" and result "Failed" would bump an issue back to 'needs work' on Drupal.org ... the "Failed to apply" patch example given above would result in a job with state "Error" instead of "Completed", and thus no status change would be triggered on that issue.

mile23’s picture

mile23
Primary language English
Location Seattle, WA
commented
Status: Needs review » Needs work

Maybe I'm reading it wrong, but this doesn't exist:
$job->getJobResults()->stages[$stage]['steps'][$step]['state'])

Instead I see this: JobResults::setStageResults which is kind of weird because you're making a distinction between 'status' and 'results,' but there's no way a stage can have a result in that sense. So we need to clarify the names of things.

Also, it seems like JobResults would encapuslate the job/stage/step results, so you'd set stage and step results as you go, and you wouldn't be able to set them all at once, which is what setStageResults() lets you do.

In addition, the tests fail, and it looks like RunCommand needs some love:

vagrant@vagrant-ubuntu-trusty-64:~/drupalci_testbot$ ./bin/phpunit 
PHPUnit 4.8.23 by Sebastian Bergmann and contributors.

EEE..............................E.....

Time: 14.19 seconds, Memory: 17.75Mb

There were 4 errors:

1) DrupalCI\Tests\Application\ContribNoTestsTest::testContribNoTests
Undefined variable: job

/home/vagrant/drupalci_testbot/src/DrupalCI/Console/Command/RunCommand.php:177
/home/vagrant/drupalci_testbot/vendor/symfony/console/Command/Command.php:259
/home/vagrant/drupalci_testbot/vendor/symfony/console/Application.php:844
/home/vagrant/drupalci_testbot/vendor/symfony/console/Application.php:192
/home/vagrant/drupalci_testbot/vendor/symfony/console/Application.php:123
/home/vagrant/drupalci_testbot/vendor/symfony/console/Tester/ApplicationTester.php:76
/home/vagrant/drupalci_testbot/tests/DrupalCI/Tests/Application/ContribNoTestsTest.php:45

2) DrupalCI\Tests\Application\PassingSimpletestTest::testBasicTest
Undefined variable: job

/home/vagrant/drupalci_testbot/src/DrupalCI/Console/Command/RunCommand.php:177
/home/vagrant/drupalci_testbot/vendor/symfony/console/Command/Command.php:259
/home/vagrant/drupalci_testbot/vendor/symfony/console/Application.php:844
/home/vagrant/drupalci_testbot/vendor/symfony/console/Application.php:192
/home/vagrant/drupalci_testbot/vendor/symfony/console/Application.php:123
/home/vagrant/drupalci_testbot/vendor/symfony/console/Tester/ApplicationTester.php:76
/home/vagrant/drupalci_testbot/tests/DrupalCI/Tests/Application/PassingSimpletestTest.php:41

3) DrupalCI\Tests\Application\PatchFailTest::testBadPatch
Undefined variable: job

/home/vagrant/drupalci_testbot/src/DrupalCI/Console/Command/RunCommand.php:177
/home/vagrant/drupalci_testbot/vendor/symfony/console/Command/Command.php:259
/home/vagrant/drupalci_testbot/vendor/symfony/console/Application.php:844
/home/vagrant/drupalci_testbot/vendor/symfony/console/Application.php:192
/home/vagrant/drupalci_testbot/vendor/symfony/console/Application.php:123
/home/vagrant/drupalci_testbot/vendor/symfony/console/Tester/ApplicationTester.php:76
/home/vagrant/drupalci_testbot/tests/DrupalCI/Tests/Application/PatchFailTest.php:40

4) DrupalCI\Tests\Console\Command\RunCommandTest::testRun
Undefined variable: job

/home/vagrant/drupalci_testbot/src/DrupalCI/Console/Command/RunCommand.php:177
/home/vagrant/drupalci_testbot/vendor/symfony/console/Command/Command.php:259
/home/vagrant/drupalci_testbot/vendor/symfony/console/Tester/CommandTester.php:80
/home/vagrant/drupalci_testbot/tests/DrupalCI/Tests/Console/Command/RunCommandTest.php:23

FAILURES!
Tests: 39, Assertions: 42, Errors: 4.
vagrant@vagrant-ubuntu-trusty-64:~/drupalci_testbot$
Mixologic’s picture

Mixologic
He/Him/His
Primary language English
Location Portland, OR
commented
Component: Code » Artifacts and Output
Mixologic’s picture

Mixologic
He/Him/His
Primary language English
Location Portland, OR
commented
Title: [meta] Complete implementation of the JobResults class » Create the BuildResults
Issue summary: View changes
Parent issue: » #2811773: [meta] Test Runner Re-Architecting

Updating the issue summary. The build as a whole needs a results file that d.o. can consume.

We'll need to think about structure such that d.o. might need to aggregate data together. (i.e. if we run multiple simpletest jobs, and each has its own pass fail numbers, how should d.o. report that? Does d.o. still get that from the jenkins api?)

  • jthorson committed 32d7a17 on 2655718-build-status-file 
    [#2655718] by jthorson: Introduce postprocess step outside of the main...
  • Mixologic committed 839b961 on 2655718-build-status-file
    Issue #2655718: Adds a build status file that either contains "Build...
Mixologic’s picture

Mixologic
He/Him/His
Primary language English
Location Portland, OR
commented
Status: Needs work » Needs review

This is actually quite simple now. Any BuildTask that fails the build with an exception throws that exception, and the message is stored as a json file in the artifacts directory, otherwise it says "Build Successful".

In project_issue_file_test/pift_ci_job.inc->requestResult(), we can look for this file first, and if anything other than "Build Successful" message is not there (i.e. "Patch did not apply"), we can use that for the message, otherwise we can process whatever testresults seem relevant (currently /testReport/api/json for simpletest etc).

  • Mixologic committed 2dece01 on 2655718-build-status-file 
    Issue #2655718: allows child status codes to bubble up to parent...
  • Mixologic committed 89f6fe8 on 2655718-build-status-file
    Issue #2655718: moves most placeholdery whatnot to the PluginBase,...
Mixologic’s picture

Mixologic
He/Him/His
Primary language English
Location Portland, OR
commented
Status: Needs review » Fixed

This has been merged into dev.

Status: Fixed » Closed (fixed)

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