-
Notifications
You must be signed in to change notification settings - Fork 180
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 support for 'loading' unit tests, which evaluate a LOADING phase. #347
Conversation
This is a relatively simple addition to unittest that statically creates rules that either explicitly fail or not depending on if the test case, evaluated entirely in loading phase, resulted in the expected state or not.
lib/unittest.bzl
Outdated
def _loading_suite(env): | ||
"""Creates a test suite for loading phase tests. | ||
|
||
Args: | ||
env: env created with loadingtest.make | ||
|
||
Returns: | ||
None, creates test_suite | ||
""" | ||
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.
This function is no-op. Was it left over from a previous iteration of the PR?
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 originally created it for consistency with the others, but its unused so yes, removed.
lib/unittest.bzl
Outdated
|
||
loadingtest = struct( | ||
make = _loading_make, | ||
asserts = _loading_asserts, |
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.
Nit: please rename to "assert" and "_loading_assert", not "asserts", for consistency with the asserts
struct defined above (in other words, we want assert
to be a method and asserts
to be a struct/namespace).
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 can't do this. assert is a keyword in starlark and fails. For now I've renamed to 'equals' to mirror the function in 'asserts'
I'm open to other suggestions.
Failures appear to be unrelated to my changes. |
For the failures - see #349 (new buildifier release lints for more things). Maybe you can review? |
I approved, but I don't think I have the appropriate project permissions to have that allow the submit. |
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.
LGTM - just please rerun docs/regenerate_docs.sh
to update unittest_doc.md :)
Whoops, done thanks for the reminder. |
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!
This is a relatively simple addition to unittest that statically creates
rules that either explicitly fail or not depending on if the test case is valid during LOADING phase of bazel. The test conditions are evaluated entirely in loading phase, but if we want an actual test to fail rather than just
fail()
killing the build, we can use this to assert state and report test failures.