Skip to content

chore(vars): using go testing entities for test helpers - #785

Open
Jacobbrewer1 wants to merge 1 commit into
gorilla:mainfrom
Jacobbrewer1:chore/using-test-utils
Open

chore(vars): using go testing entities for test helpers#785
Jacobbrewer1 wants to merge 1 commit into
gorilla:mainfrom
Jacobbrewer1:chore/using-test-utils

Conversation

@Jacobbrewer1

Copy link
Copy Markdown

What type of PR is this? (check all applicable)

  • Refactor
  • Feature
  • Bug Fix
  • Optimization
  • Documentation Update
  • Go Version Update
  • Dependency Update

Description

This pull request refactors the SetURLVars test helper to improve testing practices and updates its usage in tests. The main focus is to enhance test reliability by leveraging Go's testing.T helper methods and to update the relevant test case accordingly.

Test helper improvements:

  • Updated the SetURLVars function in test_helpers.go to accept a *testing.T parameter and call t.Helper(), improving error reporting and aligning with Go testing conventions.
  • Added necessary imports to test_helpers.go to support the new parameter.

Test case updates:

  • Replaced the ExampleSetURLVars example function in mux_test.go with a proper test function TestExampleSetURLVars that uses the updated SetURLVars helper and asserts the expected value using t.Errorf.

Added/updated tests?

  • Yes
  • No, and this is why: please replace this line with details on why tests
    have not been included
  • I need help with writing tests

Run verifications and test

  • make verify is passing
  • make test is passing

@themavik themavik left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Exported SetURLVars is part of the public API; threading *testing.T breaks every out-of-tree caller. nit: keep SetURLVars(r, val) and add a thin MustSetURLVars/SetURLVarsForTest wrapper that calls t.Helper() without changing the stable signature.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants