-
Notifications
You must be signed in to change notification settings - Fork 12
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 base structure for testing of statuses from real cloud #334
Add base structure for testing of statuses from real cloud #334
Conversation
f244ef3
to
78cc044
Compare
Add mocked-plans tox environment and job in GitHub workflows. I also remove usage of boots-actions, since it's old and not maintained.
@gabrielcocenza Thanks for doing #341. I merged, so I can use it here. |
5380e2d
to
7998cf5
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.
This is a partial review since I haven't finish going through all the changes yet.
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.
Mostly looks good with some minor changes needed
Co-authored-by: TQ X <[email protected]>
Co-authored-by: TQ X <[email protected]>
Co-authored-by: TQ X <[email protected]>
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, thanks
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.
Thank you, I really like this work!
I have only left some non-blocking suggestions.
Additionally and as a sidenote, I don't like that we're tightly coupling what a plan is to its string representation; we should be able to change how a plan is represented without having to alter the tests that verify that it is correct. But of course this cannot be solved in this PR.
I want to merge this to ublock another tasks.
With these changes, we can simply add any result form real cloud to
tests/unit/sample_plans
directory in yaml file formatted like this:My only two concerns are:
- usage ofAfter discussion I choose to got with only fixture returning dictionary with file as key, and tuple value with model and exp_plan. The model.get_applications returns applications defined in file.pytest-subtests
, which result could be strange if test failed, as you can see in current CI- these test should be more part of functional tests instead of unit tests, or somehow exclude them from coverage report (e.g. 100% coverage is achieved without these tests)After discussions we agreed that new tox env should be created, so I createdmocked-plans
. I also add new jobs to run this environment to our GitHub workflow.