-
-
Notifications
You must be signed in to change notification settings - Fork 553
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
Added more information to error when drive cycle starts at t>0 #3756
Conversation
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.
Just a small comment, but looks good overall. Can you also please add a line to the CHANGELOG?
@R-Yash I hope you don't mind, but I synced with develop so it would be ready for merging |
@kratman Thanks |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #3756 +/- ##
========================================
Coverage 99.59% 99.59%
========================================
Files 258 258
Lines 20823 20827 +4
========================================
+ Hits 20738 20742 +4
Misses 85 85 ☔ View full report in Codecov by Sentry. |
@R-Yash Can you add a test to cover that exception? |
@kratman How do I do that? I am a bit new to testcases. |
You need to make a test that calls the function where the error is located, and passes in a time that is not zero. You can look at existing tests for that function to see if there is something you can adapt |
@kratman
|
Usually it is good to write a new test. A test like a function should have a single purpose. In this case you just want to test that an error is thrown. It will probably require a "with" statement looking for the thrown exception. If you are really stuck then we can discuss on slack, but I recommend you give it a shot on your own first |
@kratman I have added a test. Please take a look |
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.
Seems good as long as the tests pass
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.
Thanks @R-Yash Looks good, Just a small changelog suggestion.
Also waiting on tests before merging.
Co-authored-by: Arjun <[email protected]>
Description
When a drive cycle starts at t>0, the current error thrown seems confusing. The changes made gives drive cycle a name and throws relevant exception when t>0. The documentation is changed accordingly to reflect this fact
Fixes #3612
Type of change
Please add a line in the relevant section of CHANGELOG.md to document the change (include PR #) - note reverse order of PR #s. If necessary, also add to the list of breaking changes.
Key checklist:
$ pre-commit run
(or$ nox -s pre-commit
) (see CONTRIBUTING.md for how to set this up to run automatically when committing locally, in just two lines of code)$ python run-tests.py --all
(or$ nox -s tests
)$ python run-tests.py --doctest
(or$ nox -s doctests
)You can run integration tests, unit tests, and doctests together at once, using
$ python run-tests.py --quick
(or$ nox -s quick
).Further checks: