Skip to content

Conversation

@pmeier
Copy link
Contributor

@pmeier pmeier commented Mar 6, 2023

Compared to CircleCI, GitHub Actions has one major DevX downside: it is really hard to find failing tests in the logs. This happens for two reasons:

  1. GitHub Actions seems to be really slow to load logs in general. We are often waiting multiple tens of seconds, with the browser already asking if we want to stop loading the side completely, because it seems to have hanged.
  2. GitHub Actions loads the lines from the beginning, while the failing tests are summarized at the end. Meaning, we spent quite some time to load some stuff that we don't want to look at.

CircleCI does this better:

  1. In case the log exceeds 400k characters, the online view will be truncated to the last 400k characters, which comes down to roughly 3k lines. Paired with a lot faster loading in general, we can see the traceback of failing tests on CircleCI before GitHub Actions even loaded the logs.
  2. CircleCI has the ability to surface failing tests in general so one doesn't even have to take a look at the logs in 99% of the cases.

Since we can't do anything about 1. here, this PR goes after 2. to bring some of the DevX from CircleCI to GitHub Actions.

This ability of CircleCI to surface failing tests comes from this builtin functionality: https://circleci.com/docs/collect-test-data/

This works by analyzing a JUnit XML file. This file can be generated by pytest, which is what the domain libraries are using to run their test suites. Copying from the CircleCI documentation, this is minimal example:

      - run:
          name: run tests
          command: |
            . venv/bin/activate
            mkdir test-results
            pytest --junitxml=test-results/junit.xml

      - store_test_results:
          path: test-results

This PR introduces a small custom GitHub action that does the same as the store_test_results from CircleCI: pytest-results-action. All it does, is to parse the same JUnit XML file and store the results as part of the job summary. The usage is the same:

- name: Run tests
  run: pytest --junit-xml=test-results.xml

- name: Summarize test results
  if: always()
  uses: pmeier/[email protected]
  with:
    junit-xml: test-results.xml

The branch of this PR was used in pytorch/vision#7364 and "stamped" by the torchvision maintainers. Offline, other domain library maintainers also expressed their support for this.

Another option currently explored by @DanilBaibak is to bring the full PyTorch HUD including the bot to the domain libraries. This seems like a larger task that might take some time. In the meantime, this PR could drastically improve DevX without wait time.

@vercel
Copy link

vercel bot commented Mar 6, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

1 Ignored Deployment
Name Status Preview Comments Updated
torchci ⬜️ Ignored (Inspect) Visit Preview Mar 13, 2023 at 1:07PM (UTC)

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Mar 6, 2023
@osalpekar
Copy link
Member

Thanks - stamping to unblock! @DanilBaibak - when do you think we can land a v1 of ci-bot on vision PRs?

@osalpekar
Copy link
Member

Thanks - stamping to unblock! @DanilBaibak - when do you think we can land a v1 of ci-bot on vision PRs?

Ah looks like #3842 is up - awesome!

@pmeier
Copy link
Contributor Author

pmeier commented Mar 13, 2023

The last few commits implement the suggestion from #3841 (comment). The only thing users will have to do is to replace the regular pytest invocation with pytest --junit-xml="${RUNNER_TEST_RESULTS_DIR}/test-results.xml" where the actual file name is arbitrary.

@pmeier pmeier requested a review from seemethere March 13, 2023 13:09
Copy link
Member

@seemethere seemethere left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thank you for incorporating the feedback! 😄

@seemethere seemethere merged commit c4de08e into main Mar 13, 2023
@pmeier pmeier deleted the test-results branch March 13, 2023 22:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants