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

chore: adapt Makefile command to run unit tests #2072

Merged
merged 5 commits into from
Sep 19, 2023
Merged

Conversation

odubajDT
Copy link
Contributor

No description provided.

@netlify
Copy link

netlify bot commented Sep 11, 2023

Deploy Preview for keptn-lifecycle-toolkit ready!

Name Link
🔨 Latest commit f3cc13c
🔍 Latest deploy log https://app.netlify.com/sites/keptn-lifecycle-toolkit/deploys/64fecf7e30f10c0008f8e211
😎 Deploy Preview https://deploy-preview-2072--keptn-lifecycle-toolkit.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@codecov
Copy link

codecov bot commented Sep 11, 2023

Codecov Report

Merging #2072 (b92529d) into main (bd15001) will decrease coverage by 24.84%.
The diff coverage is n/a.

Additional details and impacted files
@@             Coverage Diff             @@
##             main    #2072       +/-   ##
===========================================
- Coverage   83.98%   59.14%   -24.84%     
===========================================
  Files         150       35      -115     
  Lines        9597     2389     -7208     
===========================================
- Hits         8060     1413     -6647     
+ Misses       1246      836      -410     
+ Partials      291      140      -151     

see 127 files with indirect coverage changes

Flag Coverage Δ
certificate-operator 68.55% <ø> (ø)
lifecycle-operator ?
metrics-operator ?
scheduler 32.12% <ø> (ø)

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

Signed-off-by: odubajDT <[email protected]>
@github-actions github-actions bot added the ops label Sep 11, 2023
@odubajDT odubajDT changed the title chore: introduce Makefile wrapper to run unit tests chore: adapt Makefile command to run unit tests Sep 11, 2023
.github/workflows/CI.yaml Outdated Show resolved Hide resolved
Signed-off-by: odubajDT <[email protected]>
@odubajDT odubajDT force-pushed the chore/unit-test-wrapper branch from 0f323df to fa6a435 Compare September 11, 2023 08:51
Signed-off-by: odubajDT <[email protected]>
@sonarcloud
Copy link

sonarcloud bot commented Sep 11, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

Copy link
Member

@thisthat thisthat left a comment

Choose a reason for hiding this comment

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

I would not introduce this change since they could be anything and not necessary just unit tests. At the end, the current entry is a wrapper around go test, hence make test is a better name imho.
If you'd like to have a "run local tests before creating a PR", I'd rather have a new Makefile entry in the root Makefile with a unit-test step that calls all the test entrypoint in the respective sub-folders.

@odubajDT
Copy link
Contributor Author

odubajDT commented Sep 11, 2023

I would not introduce this change since they could be anything and not necessary just unit tests. At the end, the current entry is a wrapper around go test, hence make test is a better name imho. If you'd like to have a "run local tests before creating a PR", I'd rather have a new Makefile entry in the root Makefile with a unit-test step that calls all the test entrypoint in the respective sub-folders.

I would argue here. Currently make test runs only unit tests, but for me as a user it sounds like it runs all the tests (what is not true). We also use make test command as part of unit-tests pipeline, which speaks for itself. I think this was just named wrongly in the past. If we look at it from a higher perspective (for example for lifecycle-operator) we have multiple makefile entries:

  • component-test
  • e2e-test
  • integration-test
  • test (original name, in this PR rename to unit-test)

This for me makes logical sense.

About the second though of introducing a unit-test entry in the root Makefile -> I would be against this. In most cases, developers are adapting only one component of the toolkit (at least this should be true for the majority of PRs), so metrics-operator, lifecycle-operator, ... It does not make any sense to have the possibility to run unit tests for all components, when you are adapting only a single one. For advanced tests (like integration-tests) it surely does, but for simple unit-tests not.

@RealAnna
Copy link
Contributor

I would not introduce this change since they could be anything and not necessary just unit tests. At the end, the current entry is a wrapper around go test, hence make test is a better name imho. If you'd like to have a "run local tests before creating a PR", I'd rather have a new Makefile entry in the root Makefile with a unit-test step that calls all the test entrypoint in the respective sub-folders.

I would argue here. Currently make test runs only unit tests, but for me as a user it sounds like it runs all the tests (what is not true). We also use make test command as part of unit-tests pipeline, which speaks for itself. I think this was just named wrongly in the past. If we look at it from a higher perspective (for example for lifecycle-operator) we have multiple makefile entries:

  • component-test
  • e2e-test
  • integration-test
  • test (original name, in this PR rename to unit-test)

This for me makes logical sense.

About the second though of introducing a unit-test entry in the root Makefile -> I would be against this. In most cases, developers are adapting only one component of the toolkit (at least this should be true for the majority of PRs), so metrics-operator, lifecycle-operator, ... It does not make any sense to have the possibility to run unit tests for all components, when you are adapting only a single one. For advanced tests (like integration-tests) it surely does, but for simple unit-tests not.

Also OFO does use unit-test https://github.com/open-feature/open-feature-operator/blob/main/Makefile and same goes for prometheus operator where test calls all tests including unit-tests https://github.com/prometheus-operator/prometheus-operator/blob/main/Makefile#L338

@odubajDT odubajDT merged commit 2db2569 into main Sep 19, 2023
32 of 33 checks passed
@odubajDT odubajDT deleted the chore/unit-test-wrapper branch September 19, 2023 12:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants