Skip to content
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

Remove stringify tests #2122

Merged
merged 1 commit into from
Jul 9, 2024
Merged

Remove stringify tests #2122

merged 1 commit into from
Jul 9, 2024

Conversation

kislaykishore
Copy link
Collaborator

Description

These tests attempt to test the behavior of stringify method but in reality don't serve much purpose for the following reasons:

  • This test doesn't really test whether the util.Stringify was called.
  • They test the behavior of util.Stringify but there are tests in util package that do that here.
  • They are usually a nuisance while adding flags as the tests break on flag addition.

Link to the issue in case of a bug fix.

NA

Testing details

  1. Manual - NA
  2. Unit tests - NA
  3. Integration tests - NA

@kislaykishore kislaykishore requested a review from a team as a code owner July 8, 2024 09:31
Copy link

codecov bot commented Jul 8, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 71.73%. Comparing base (cecf4ac) to head (a823d1a).

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2122      +/-   ##
==========================================
- Coverage   71.75%   71.73%   -0.02%     
==========================================
  Files         101      101              
  Lines       11120    11120              
==========================================
- Hits         7979     7977       -2     
- Misses       2812     2814       +2     
  Partials      329      329              
Flag Coverage Δ
unittests 71.73% <ø> (-0.02%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

ankitaluthra1
ankitaluthra1 previously approved these changes Jul 8, 2024
Copy link
Collaborator

@ankitaluthra1 ankitaluthra1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"stringify" should not test actual flag being passed, that not part of stringify behaviour. that should be checked in integration or composite tests @kislaykishore please also add reference to any of the integration or composite test testing that behaviour in PR description for reference.

ashmeenkaur
ashmeenkaur previously approved these changes Jul 9, 2024
These tests attempt to test the behavior of stringify method but in
reality don't serve much purpose for the following reasons:
* This test doesn't really test whether the util.Stringify was called.
* They test the behavior of util.Stringify but there are tests in util
  package that do that
  [here](https://github.com/GoogleCloudPlatform/gcsfuse/blob/2fb8bba983cd81270357bfc590ccb0d5b4e6fb76/internal/util/util_test.go#L151).
* It's usually a nuisance while adding flags where the test breaks on
  new flag addition.
@kislaykishore
Copy link
Collaborator Author

"stringify" should not test actual flag being passed, that not part of stringify behaviour. that should be checked in integration or composite tests @kislaykishore please also add reference to any of the integration or composite test testing that behaviour in PR description for reference.

Created #2134 to create such an integration test.

@kislaykishore kislaykishore merged commit 22dc19e into master Jul 9, 2024
13 checks passed
@kislaykishore kislaykishore deleted the stringify-test-remove branch July 9, 2024 12:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants