-
Notifications
You must be signed in to change notification settings - Fork 37
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 explicit pathwaysutils.initialize() call to JetStream #233
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.
I think this is less of an "error" and more of "we are running jetstream without pathways. Could we clarify too that this could be intended behavior, or else users may think that there is some failure happening.
2fba2da
to
ddf43ec
Compare
Thanks. Updated the log to "Running JetStream without Pathways. Module pathwaysutils is not imported." Please review again. |
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.
looks good, can you fix the lint issue?
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 for the fixes, lgtm!
ddf43ec
to
8d385b6
Compare
It's weird that it could not pass the checks (pyink format check & test coverage report). I don't see a quick fix due to lack of change suggestions. Shall we bypass the checks? |
pyink you can fix by running |
8d385b6
to
1e2dfad
Compare
Thank you. Could you also guide me how to add test coverage for |
You could probably write a test expecting the error. |
b03b5c6
to
b1a70eb
Compare
@vipannalla I added the unit test for module engine init. This unit test can pass by running separately |
9fa4fb8
to
596019c
Compare
596019c
to
b8c1b7d
Compare
b8c1b7d
to
46247f0
Compare
Its complaining about unit test coverage for a test file, which should really be excluded from the check. |
Description
pathwaysutils will soon require a call to initialize().
Tests
Depending on GitHub actions for tests
Checklist
Before submitting this PR, please make sure (put X in square brackets):