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

RIP Candid - Follow up #1174

Merged
merged 2 commits into from
Mar 18, 2024
Merged

Conversation

kian99
Copy link
Contributor

@kian99 kian99 commented Mar 12, 2024

Description

Follow up on previous PR comments, addressing 3 issues:

  • Use a checker in NewUserSessionLogin to fail the test instead of a panic.
  • Added Godoc to NewUserSessionLogin.
  • Fixed incorrect string in internal/jimm/access_test.go

@kian99 kian99 changed the title Improve test setup RIP Candid - Follow up Mar 12, 2024
Copy link
Collaborator

@alesstimec alesstimec left a comment

Choose a reason for hiding this comment

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

LGTM

// Useful for enabling test setup functions to fail without a panic.
type SimpleTester interface {
Fatalf(format string, args ...interface{})
Logf(format string, args ...interface{})
Copy link
Collaborator

Choose a reason for hiding this comment

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

nitpick: we're not using Logf anywhere..

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah I just used the Tester interface and left in only what both the GoCheck and QuickTest testers implement. Might be handy in the future at a glance.

Copy link
Member

@babakks babakks Mar 18, 2024

Choose a reason for hiding this comment

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

We already have one in internal/jimmtest/gorm.go. I think you can safely use that:

// A Tester is the test interface required by this package.
type Tester interface {
	Fatalf(format string, args ...interface{})
	Logf(format string, args ...interface{})
	Name() string
	Cleanup(f func())
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried to but GoCheck doesn't implement all of those so instead of using the wrapper struct I just made a SimpleTester interface instead. In most places we have a GoCheck tester and in some places a qt tester and I didn't want to change the caller code to wrap the GoCheck tester in a struct before passing it.

// Useful for enabling test setup functions to fail without a panic.
type SimpleTester interface {
Fatalf(format string, args ...interface{})
Logf(format string, args ...interface{})
Copy link
Member

@babakks babakks Mar 18, 2024

Choose a reason for hiding this comment

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

We already have one in internal/jimmtest/gorm.go. I think you can safely use that:

// A Tester is the test interface required by this package.
type Tester interface {
	Fatalf(format string, args ...interface{})
	Logf(format string, args ...interface{})
	Name() string
	Cleanup(f func())
}

@kian99 kian99 merged commit 12c20db into canonical:feature-oidc Mar 18, 2024
4 checks passed
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