-
Notifications
You must be signed in to change notification settings - Fork 244
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
Uncomment odo project list json test #3546
Uncomment odo project list json test #3546
Conversation
c62d778
to
c2737e0
Compare
c2737e0
to
729f5e9
Compare
Expect(err).Should(BeNil()) | ||
partOfProjectListJSON, err := helper.Unindented(`{"kind":"Project","apiVersion":"odo.dev/v1alpha1","metadata":{"name":"` + project + `","namespace":"` + project + `","creationTimestamp":null},"spec":{},"status":{"active":true}}`) | ||
Expect(err).Should(BeNil()) | ||
Expect(listOutputJSON).To(ContainSubstring(partOfProjectListJSON)) |
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.
It wold be better to use MatchJSON
instead of ContainSubstring
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.
@kadel yes, this a better option but it won't work the test scenario we are verifying. We are not matching the whole json here, some part of the json is being matched with the original output because projectListJSON := helper.CmdShouldPass("odo", "project", "list", "-o", "json")
returns a huge output in CI and in locally the size depends on how many projects are there in that cluster when you ran the test script.
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.
Agreed, match json might not work as we are comparing a substring not the whole json. But then my question is why not? Why not compare the whole json output using MatchJSON seems more robust to me?
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.
But then my question is why not? Why not compare the whole json output using MatchJSON seems more robust to me?
@girishramnani json length is not the only concern, the major concern is the json length is dynamic. For example namespace myproject
and ci-operator-hub-project
are two default project we get when we run our CI test. So may be down the line we can add more namespace through the configure script. This can lead to change in json length.
The worst part we will face when we run the test script locally. For example you have a cluster and you running your test as kebeadmin
then helper.CmdShouldPass("odo", "project", "list", "-o", "json")
will list out almost 30 project details. It is weird to add a huge json list to match in test script.
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.
So to over come this i am just matching the key portion of the json list
/lgtm |
/retest |
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
/approve
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: mohammedzee1000 The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
/retest Please review the full test history for this PR and help us cut down flakes. |
1 similar comment
/retest Please review the full test history for this PR and help us cut down flakes. |
Codecov Report
@@ Coverage Diff @@
## master #3546 +/- ##
=======================================
Coverage 46.27% 46.27%
=======================================
Files 112 112
Lines 11386 11386
=======================================
Hits 5269 5269
Misses 5608 5608
Partials 509 509 Continue to review full report at Codecov.
|
What type of PR is this?
/kind cleanup
What does does this PR do / why we need it:
Uncomment odo project list json test
Which issue(s) this PR fixes:
Fixes #1708
How to test changes / Special notes to the reviewer:
make test-cmd-project