Skip to content

Add unit tests for CLI and Auth packages - #55

Merged
tonymeehan merged 4 commits into
prequel-dev:mainfrom
amanycodes:cli-coverage-test
Jun 8, 2025
Merged

tonymeehan merged 4 commits into
prequel-dev:mainfrom
amanycodes:cli-coverage-test

Conversation

@amanycodes

@amanycodes amanycodes commented Jun 7, 2025

Copy link
Copy Markdown
Contributor

Added unit and integration tests for cli focusing to:

  • Test parseSources with valid, invalid an empty data-source files.
  • Mock rules.GetRules and auth.Login using monkey-patching in InitAndExecute to confirm option parsing.

Since there was no dependency injection I couldn't use interfaces for rules.GetRules and auth.Login, so I wrote some package-level function variables for both helping me directly mock them in the test file and isolate them from other (network, fs, etc) dependencies.

Also. while writing tests I saw that the current datasrc.Validate() and datasrc.Parsefile() are more permissive and do not currently return errors for cases like: missing source key (considers file just with version: 1.0) and invalid source type (doesn't check the type field)

I'll add these checks to the datasrc package once these are confirmed.

Looking forward for feedback and review! Thanks!

Partially closes: #48
cc @tonymeehan

Signed-off-by: amanycodes <amanycodes@gmail.com>
Signed-off-by: amanycodes <amanycodes@gmail.com>
@amanycodes

Copy link
Copy Markdown
Contributor Author

Also cc: @aleksmaus @scunningham

Signed-off-by: amanycodes <amanycodes@gmail.com>
Signed-off-by: amanycodes <amanycodes@gmail.com>
@amanycodes amanycodes changed the title Add unit tests for CLI package Add unit tests for core packages Jun 7, 2025
Comment thread internal/pkg/cli/cli.go

// Log in for community rule updates
if token, err = auth.Login(ctx, baseAddr, ruleToken); err != nil {
if token, err = loginUserFunc(ctx, baseAddr, ruleToken); err != nil {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Definitely a matter of preference, but let's leave these function calls referencing the package instead of using the helper functions. Prefer to see dependencies immediately as we read the code, so seeing auth.X() or rules.Y() is helpful.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Definitely a matter of preference, but let's leave these function calls referencing the package instead of using the helper functions. Prefer to see dependencies immediately as we read the code, so seeing auth.X() or rules.Y() is helpful.

I agree with the explicitness being more readable! But actually when I started the tests for initAndExecute I couldn't do direct calls to auth.Login and rules.GetRules since that was then depending on reaching to networks and I was having trouble as they were requiring complex setup. I would love your feedback on how to do this without the variable definition. Thanks!

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Ah that makes sense. I missed you were mocking them in the tests. All good. Can you leave comments above each call just noting why we're doing this? and note the original call in the comment?

@tonymeehan tonymeehan left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks good. Please fix up the function name helpers in cli.go and then we'll merge!

@amanycodes

Copy link
Copy Markdown
Contributor Author

Added unit and integration tests for auth to:

  • Validate emailClaim with valid, invalid, malformed and missing JWT segments.
  • Unit test checkLocalToken with expired token, malformed token, token signed with wrong key, token doesn't exists.
  • Login using Localtoken and complete flow.

@amanycodes

amanycodes commented Jun 7, 2025

Copy link
Copy Markdown
Contributor Author

@tonymeehan The changes are getting large, should I raise another one for the upcoming tests?

@amanycodes amanycodes changed the title Add unit tests for core packages Add unit tests for CLI and Auth packages Jun 7, 2025
@tonymeehan

Copy link
Copy Markdown
Contributor

@tonymeehan The changes are getting large, should I raise another one for the upcoming tests?

Good idea. Let's do that. This is looking good, though. Merging now. Nice work improving the coverage!

@tonymeehan
tonymeehan merged commit 12da462 into prequel-dev:main Jun 8, 2025
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.

Add Unit Tests for core packages

2 participants