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

tests: Add testing for xdsl_opt_main #290

Merged
merged 2 commits into from Jan 9, 2023
Merged

tests: Add testing for xdsl_opt_main #290

merged 2 commits into from Jan 9, 2023

Conversation

webmiche
Copy link
Collaborator

@webmiche webmiche commented Jan 9, 2023

This PR hacks together a way to test xdsl_opt_main through some functionality of the arg parser.

Overall, I am feeling like the split of execution models (in a python interpreter vs from command line) will always come back to haunt us. I am not sure whether we actually can build something cleanly without reimplementing lit essentially.

@webmiche webmiche added the testing PRs that modify tests label Jan 9, 2023
@webmiche webmiche self-assigned this Jan 9, 2023
@webmiche webmiche changed the title tests: add testing for xdsl_opt_main tests: Add testing for xdsl_opt_main Jan 9, 2023
@codecov
Copy link

codecov bot commented Jan 9, 2023

Codecov Report

Base: 86.60% // Head: 88.77% // Increases project coverage by +2.16% 🎉

Coverage data is based on head (826166a) compared to base (e750bd0).
Patch coverage: 100.00% of modified lines in pull request are covered.

❗ Current head 826166a differs from pull request most recent head c3c8c1b. Consider uploading reports for the commit c3c8c1b to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #290      +/-   ##
==========================================
+ Coverage   86.60%   88.77%   +2.16%     
==========================================
  Files          47       48       +1     
  Lines        6586     6600      +14     
  Branches     1107     1108       +1     
==========================================
+ Hits         5704     5859     +155     
+ Misses        650      505     -145     
- Partials      232      236       +4     
Impacted Files Coverage Δ
tests/xdsl_opt/test_xdsl_opt.py 100.00% <100.00%> (ø)
xdsl/xdsl_opt_main.py 77.64% <100.00%> (+57.64%) ⬆️
xdsl/irdl_mlir_printer.py 92.04% <0.00%> (+22.72%) ⬆️
xdsl/dialects/cmath.py 85.18% <0.00%> (+85.18%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

Copy link
Contributor

@georgebisbas georgebisbas left a comment

Choose a reason for hiding this comment

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

This manages to get the cov for the python files through these tests.
Was that intentional?

@webmiche
Copy link
Collaborator Author

webmiche commented Jan 9, 2023

Yes, it is the main intention of this PR to get the coverage for the xdsl_opt_main file.

@georgebisbas
Copy link
Contributor

Yes, it is the main intention of this PR to get the coverage for the xdsl_opt_main file.

Cool, I just couldn't directly infer by the title and the description.
Maybe you could edit accordingly.

Copy link
Contributor

@georgebisbas georgebisbas left a comment

Choose a reason for hiding this comment

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

Overall I am happy; just some improvement on the doc side of the usability of these files?
Trying to think as someone that has no idea why we need this driver for.

@@ -0,0 +1,7 @@
builtin.module() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe add some comments/dosctring at the start of the file?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That's actually impossible/hacky because I compare to this file later on.

Do you think a README in this folder would be enough?

Copy link
Collaborator

@math-fehr math-fehr 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!

Copy link
Collaborator

@math-fehr math-fehr left a comment

Choose a reason for hiding this comment

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

Perfect! Thanks!

@math-fehr math-fehr merged commit e673e2c into main Jan 9, 2023
@math-fehr math-fehr deleted the michel/xdsl-opt-cov branch January 9, 2023 16:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
testing PRs that modify tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants