-
Notifications
You must be signed in to change notification settings - Fork 674
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
Make status test readable #2940
Make status test readable #2940
Conversation
c6a5d36
to
5718a5b
Compare
Codecov Report
@@ Coverage Diff @@
## main #2940 +/- ##
=======================================
Coverage 74.91% 74.91%
=======================================
Files 87 87
Lines 5604 5604
=======================================
Hits 4198 4198
Misses 1315 1315
Partials 91 91 |
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.
The main things here are that we should establish a consistent naming convention for fixture globals, and use a local run
helper in the status tests. I haven't reviewed the test coverage in detail; the coverage percentage is unchanged, which is great, but we may want to ensure that we didn't drop important test cases.
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.
couple nits so far; will look through status_test.go in some more detail.
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.
Other than the nits I'm happy with this, it's definitely an improvement in readability. I agree with James' comment that we may want to do a detailed tick and tie to ensure we didn't inadvertently drop any important test cases, but I haven't done that.
I only knowingly removed two test cases, that I checked carefully to ensure that were duplicates. I'll use the "show coverage" thing on I'll also swap over to the run style rather than table style; that's not a huge change now the hard part (moving everything) is done. |
- Extract status Secret and Service fixtures out to `internal/fixture`, assuming they could be reused. Signed-off-by: Nick Young <ynick@vmware.com>
Signed-off-by: Nick Young <ynick@vmware.com>
This commit attempts to make the status tests more readable by putting the fixtures next to the test cases they are used in as much as possible. Names are also changed. No more `proxy47a`! I also found some duplicate tests (probably from when we converted IngressRoute tests to HTTPProxy ones). Signed-off-by: Nick Young <ynick@vmware.com>
Signed-off-by: Nick Young <ynick@vmware.com>
479d60b
to
2aa688a
Compare
I've gone over all of the places in |
Signed-off-by: Nick Young <ynick@vmware.com>
Signed-off-by: Nick Young <ynick@vmware.com>
Signed-off-by: Nick Young <ynick@vmware.com>
Only test failure is the SNI flake, I'm going to go ahead and merge this so I can bring it in to the other status work. |
This PR attempts to make the status tests more readable by putting the
fixtures next to the test cases they are used in as much as possible.
internal/fixtures
.proxy47a
!Blocks #2495 until it's merged; I don't want to attempt refactoring all these tests until it's a bit
easier to see what everything's used for.
Signed-off-by: Nick Young ynick@vmware.com