-
Notifications
You must be signed in to change notification settings - Fork 9
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
Add runtime tests #475
base: main
Are you sure you want to change the base?
Add runtime tests #475
Conversation
4696db6
to
8089980
Compare
8089980
to
941752f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome! Thanks for implementing this, I think this will be really useful. I had also worked on the same thing, but was pursuing sth slightly different. I have added my proof of concept to this branch. (see also my comments). Feel free to work off of it or remove it. I think having a seperate CI for regressions might be useful in the long run.
Happy to discuss this more.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
5b0281b
to
2855ff7
Compare
eb50f1b
to
8e6de5a
Compare
8cfc276
to
824a83f
Compare
New Baselines
|
@michaeldeistler This should be ready for review. The following is implemented and works:
If you want to run this locally do you can do Some thoughts I had: Even with a 20% tolerance the regression tests frequently fail for some reason. (might need to average across several runs or take the max across several runs for the baseline, but currently the regression tests take quite long. Would be good to merge #489 to speed this up). Lemme know if anything is unclear |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wow this is amazing!!!
I don't think I understand when exactly the updating of the times is being run. On a comment to this particular PR (i.e., #475)? Or some other PR/issue? Also, what should the comment be? Anything? Who would the comment have to be by? Anyone? (BTW I am commenting now, so does this trigger the regression tests :D ?)
- name: Install dependencies | ||
run: | | ||
python -m pip install --upgrade pip | ||
pip install -e ".[dev]" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we really need dev?
- name: Install dependencies | ||
run: | | ||
python -m pip install --upgrade pip | ||
pip install -e ".[dev]" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
dev needed?
on: | ||
issue_comment: # trigger from comment; event runs on the default branch | ||
types: [created] | ||
# pull_request: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
delete?
git commit -m "Update regression test baselines" | ||
git push origin HEAD:${{ github.head_ref }} | ||
|
||
# Needed when workflow is triggered from a comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
delete?
@@ -55,6 +55,8 @@ coverage.xml | |||
*.py,cover | |||
.hypothesis/ | |||
.pytest_cache/ | |||
tests/regression_test_results.json | |||
tests/regression_test_baselines.json |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't get this. Why do we first put it in the gitignore just to then force add it?
json.dump(result_data, f, indent=2) | ||
|
||
|
||
class compare_to_baseline: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CompareToBaseline
to follow PEP style
Note that the tests are disabled in the workflow:
pytest -m "not runtime"