Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Python test report #39

Merged
merged 6 commits into from Jul 5, 2021
Merged

Python test report #39

merged 6 commits into from Jul 5, 2021

Conversation

Mandrenkov
Copy link
Collaborator

@Mandrenkov Mandrenkov commented Jun 25, 2021

Context:
The Python component of Jet has grown extensively since the GitHub Actions workflows were added to the repository. This PR aims to put the Jet Python code on equal footing with the Jet C++ code in terms of CI visibility.

Description of the Change:

  • Removed the C++ MacOS test report since it is (almost) always identical to the C++ Ubuntu test report.
  • Added a Python test report (based on the Ubuntu job).

Benefits:

  • Python test additions and failures are reflected in an auto-updated comment on each PR.

Possible Drawbacks:

  • Differences in test coverage between the Jet C++ Ubuntu and MacOS builds are more difficult to spot.

Related GitHub Issues:
None

@Mandrenkov Mandrenkov added the code quality 🎓 Improvements to code quality label Jun 25, 2021
@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions
Copy link

github-actions bot commented Jun 25, 2021

Test Report (Python) on Ubuntu

    1 files      1 suites   6s ⏱️
490 tests 490 ✔️ 0 💤 0 ❌

Results for commit d240052.

♻️ This comment has been updated with latest results.

@github-actions
Copy link

github-actions bot commented Jun 25, 2021

Test Report (C++) on Ubuntu

    1 files      1 suites   0s ⏱️
521 tests 521 ✔️ 0 💤 0 ❌
870 runs  870 ✔️ 0 💤 0 ❌

Results for commit d240052.

♻️ This comment has been updated with latest results.

@Mandrenkov Mandrenkov requested review from brownj85, mlxd and trevor-vincent and removed request for trevor-vincent June 25, 2021 19:34
@Mandrenkov Mandrenkov marked this pull request as ready for review June 25, 2021 19:36
Copy link

@jswinarton jswinarton left a comment

Choose a reason for hiding this comment

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

Looks good to me code-wise, I was just wondering if you could give a rationale for removing the C++ MacOS test report? If it's almost always identical to Ubuntu but not always, wouldn't it make it more difficult to understand problems that come up for MacOS? Is it a readability thing?

@Mandrenkov
Copy link
Collaborator Author

I was just wondering if you could give a rationale for removing the C++ MacOS test report?

Sure. I did this for two reasons:

  1. Reviewers are more likely to scrutinize the results of two test reports than four test reports.
  2. The MacOS test report doesn't add much value since we don't have any OS-specific tests. You can think of the MacOS test report as a legacy feature that made sense when Jet relied on dependencies which did not support both Linux and MacOS.

If it's almost always identical to Ubuntu but not always, wouldn't it make it more difficult to understand problems that come up for MacOS?

To clarify, I think the Ubuntu and MacOS test reports only differed when Jet failed to compile on one of the operating systems; I don't think there are any instances where both reports listed more than one test but were otherwise different.

With respect to your question, I would argue this is not a concern since any test failures on MacOS would still be reported by the corresponding GitHub Actions check. The test report action simply posts a comment on a PR stating how many tests have been added relative to the base branch and which of these tests passed or failed in the latest CI run.

Is it a readability thing?

Yes, exactly.

@Mandrenkov Mandrenkov merged commit ab7ba52 into main Jul 5, 2021
@Mandrenkov Mandrenkov deleted the python-test-report branch July 5, 2021 19:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
code quality 🎓 Improvements to code quality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants