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(deps): Updated to ginkgo v2 #2459

Merged
merged 6 commits into from
Jul 18, 2024

Conversation

kvanzuijlen
Copy link
Member

@kvanzuijlen kvanzuijlen commented Jan 24, 2024

Description

Updated ginkgo to v2

Motivation and Context

Ginkgo was outdated

How Has This Been Tested?

Checklist:

Ran tests locally.

  • My change requires a change to the documentation or CHANGELOG.
  • I have updated the documentation/CHANGELOG accordingly.
  • I have created a feature (non-master) branch for my PR.
  • I have written tests for my code changes.

I don't think this change requires a changelog entry since only _test.go files were changed

@kvanzuijlen kvanzuijlen requested a review from a team as a code owner January 24, 2024 20:04
@github-actions github-actions bot added dependencies Pull requests that update a dependency file go tests provider labels Jan 24, 2024
@JoelSpeed
Copy link
Member

Changes look good, but somehow it's broken the redis tests so will need some investigation as to why those are broken now

@kvanzuijlen
Copy link
Member Author

I'll have a look this evening

@kvanzuijlen
Copy link
Member Author

@tuunit any idea? I've been going around in circles for a couple of hours. A fresh pair of (more experienced) eyes would help.

@kvanzuijlen kvanzuijlen marked this pull request as draft February 14, 2024 09:44
@JoelSpeed
Copy link
Member

Currently showing

=== RUN   TestBasicSuite
Ginkgo detected an issue with your spec structure
AfterSuite(func() {
/home/runner/work/oauth2-proxy/oauth2-proxy/pkg/authentication/basic/htpasswd_test.go:104
  It looks like you are trying to add a [AfterSuite] node within a container
  node.

  AfterSuite can only be called at the top level.

  Learn more at:
  http://onsi.github.io/ginkgo/#suite-setup-and-cleanup-beforesuite-and-aftersuite

FAIL	github.com/oauth2-proxy/oauth2-proxy/v7/pkg/authentication/basic	0.020s

Which looks like it should be fairly straightforward, AfterSuite cannot be nested inside another container

@JoelSpeed
Copy link
Member

Should be AfterEach not AfterSuite

@tuunit
Copy link
Member

tuunit commented Mar 31, 2024

@kvanzuijlen @JoelSpeed I fixed the Ginkgo issues :)

@tuunit
Copy link
Member

tuunit commented Mar 31, 2024

Should be AfterEach not AfterSuite

Indeed this was one of the issues. Ginkgo v1 didn't really enforce that AfterSuite can only be defined outside of the spec.

Furthermore, the execution order seems to have changed slightly which causes issues with the miniredis teardown. Therefore I separated the redis store tests into two specs. One for the miniredis configure with TLS and one without.

@github-actions github-actions bot added the docs label Mar 31, 2024
@kvanzuijlen kvanzuijlen marked this pull request as ready for review March 31, 2024 12:59
Copy link
Contributor

This pull request has been inactive for 60 days. If the pull request is still relevant please comment to re-activate the pull request. If no action is taken within 7 days, the pull request will be marked closed.

@github-actions github-actions bot added the Stale label May 31, 2024
@github-actions github-actions bot closed this Jun 8, 2024
@tuunit tuunit reopened this Jun 23, 2024
@tuunit tuunit merged commit 343bd61 into oauth2-proxy:master Jul 18, 2024
6 checks passed
@tuunit tuunit deleted the ginkgo-update branch July 18, 2024 20:41
jjlakis pushed a commit to jjlakis/oauth2-proxy that referenced this pull request Oct 19, 2024
* chore(deps): Updated to ginkgo v2

* fix basic auth test suite cleanup

* fix redis store tests

* add changelog entry

---------

Co-authored-by: Jan Larwig <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Pull requests that update a dependency file docs go provider tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants