-
Notifications
You must be signed in to change notification settings - Fork 135
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
[issue-558] add test for optional feature to GitHub Action #568
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.
Good catch!
I have a question, though.
- name: Install optional dependencies | ||
run: python -m pip install networkx | ||
- name: Run tests for graph generation | ||
run: pytest tests/spdx/test_graph_generation.py |
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.
Can't we just install the optional dependencies together with the rest under "Installation"? Then we wouldn't need to call pytest again here.
By the way, is there a way to make sure that these tests actually are not skipped in the workflow?
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.
We can but I thought it would be good to test the basic installation without the optional dependencies to ensure that these tests are skipped and wouldn't cause an issue for users that don't have the optional dependencies.
Good question and good point, I will try to find out how to do this.
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.
One approach to enforce the execution of the tests would be to set an environment variable like PYTEST_DISALLOW_SKIP
and set this in the GitHub Action. Or we could check the output of pytest
if the tests were executed. As all of this seems rather complicated, I think we can postpone this issue and stick to the current GitHub Action.
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.
Ok, let's open an issue then to keep it in the backlog.
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.
0bba5ea
to
90f8cdb
Compare
Signed-off-by: Meret Behrens <meret.behrens@tngtech.com>
Signed-off-by: Meret Behrens <meret.behrens@tngtech.com>
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
I decided to add the installation of the optional dependency and the test as additional steps to the GitHub Action so that we ensure on the one hand that the source installation is successful und all tests pass (resp. are skipped) and on the other hand also run the tests for the optional feature.
addition to #562, fixes #558
Edit: I updated the test to only be skipped if
networkx
is not installed and install this package manually in the GitHub Action, as this is the only package needed for the tests.pygraphviz
requires an installation ofgraphviz
on the operating system. This seems to be more involving for Windows so I decided not to add this for the moment, as this is only an optional feature.