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

Add RegularExpressionTestUtil for testing regexes #1041

Merged
merged 11 commits into from
Jan 24, 2025

Conversation

ldhardy
Copy link
Collaborator

@ldhardy ldhardy commented Jan 7, 2025

There are multiple places we're doing something similar to test regex, and plenty of places we need to, but are not yet. It would be nice to clean it all up and use one utility class. Needed for another effort.

Copy link
Collaborator

@drivenflywheel drivenflywheel left a comment

Choose a reason for hiding this comment

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

Looks like a good addition. We should add at least cursory test coverage now to ensure that future regressions that are accidentally introduced will get caught prior to downstream integration.

ldhardy and others added 2 commits January 8, 2025 06:13
Co-authored-by: drivenflywheel <dan@drivenflywheel.com>
Co-authored-by: drivenflywheel <dan@drivenflywheel.com>
@ldhardy ldhardy requested a review from drivenflywheel January 8, 2025 11:14
@jpdahlke jpdahlke added this to the v8.21.0 milestone Jan 10, 2025
@drivenflywheel
Copy link
Collaborator

This diff looks to have gone a bit wonky. Lots of out-of-scope files now showing up as changed

@ldhardy
Copy link
Collaborator Author

ldhardy commented Jan 13, 2025

should be good now. Just one file.

@jpdahlke jpdahlke added feature A new feature that does not exist today test-only The change only impacts test code labels Jan 17, 2025
@dev-mlb dev-mlb self-requested a review January 17, 2025 16:58
Copy link
Collaborator

@dev-mlb dev-mlb left a comment

Choose a reason for hiding this comment

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

#14 67.84 /opt/emissary/src/test/java/emissary/test/util/RegularExpressionTestUtil.java:[19,7] [PrivateConstructorForUtilityClass] Classes which are not intended to be instantiated should be made non-instantiable with a private constructor. This includes utility classes (classes with only static members), and the main class.
#14 67.84     (see https://errorprone.info/bugpattern/PrivateConstructorForUtilityClass)
#14 67.84   Did you mean 'public final class RegularExpressionTestUtil {'?
#14 67.84 [WARNING] /opt/emissary/src/test/java/emissary/test/util/RegularExpressionTestUtil.java:[20,28] [NonFinalStaticField] Static fields should almost always be final.
#14 67.84     (see https://errorprone.info/bugpattern/NonFinalStaticField)

@ldhardy ldhardy requested a review from dev-mlb January 17, 2025 18:22
@ldhardy ldhardy dismissed drivenflywheel’s stale review January 22, 2025 11:42

Dismiss stale review.

@jpdahlke jpdahlke merged commit 3ec9b49 into NationalSecurityAgency:main Jan 24, 2025
12 checks passed
@jpdahlke jpdahlke changed the title Add RegularExpresionTestUtil for testing regexes Add RegularExpressionTestUtil for testing regexes Jan 24, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature A new feature that does not exist today test-only The change only impacts test code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants