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 extension / annotation for restoring system properties #574

Closed
Marcono1234 opened this issue Jan 6, 2022 · 4 comments · Fixed by #700
Closed

Add extension / annotation for restoring system properties #574

Marcono1234 opened this issue Jan 6, 2022 · 4 comments · Fixed by #700

Comments

@Marcono1234
Copy link
Contributor

Based on #381 (comment)

In some situations the existing annotations for clearing and setting system properties are not sufficient:

  • When the property name is dynamic
  • When the property value is dynamic and the currently described workaround of using @ClearSystemProperty is considered too error-prone (especially when setting multiple system properties)
  • When system properties are set outside the test method, e.g. inside the tested code

It would therefore be useful to have an annotation which captures the current system properties before the test execution (obtained via System.getProperties()), copies them and then restores them to this state after the test execution.
The name of the annotation could for example be @RestoreSystemProperties. This is the same name used by System Rules, but it is quite accurate.

Considerations / open questions:

  • Should usage of this annotation be allowed together with @ClearSystemProperty and @SetSystemProperty?
    If so, it should be checked if there are any corner cases where this could cause issues.
  • System properties can, despite being discouraged, contain non-String keys and values. It is probably not worth it add any special handling for them (such as performing deep copies), however the documentation should then document this behavior.
  • Should the annotation be allowed on class level?
    Probably, would definitely be useful.
  • When used to annotate a test class, should the annotation apply to before-all & after-all, before-each & after-each or only the test method itself (see also Enclosing classes vs Before/AfterAll #479)?
@beatngu13
Copy link
Member

@nipafx
Copy link
Member

nipafx commented Apr 28, 2022

This is a good proposal, I like it. Generally speaking, the system property extension already stores and resets extensions so we can probably reuse some of that machinery.

@RestoreSystemProperties would have to back up and restore all system properties. Maybe an easier first stab at the problem would be @RestoreSystemProperty("foo").

To your questions:

  • I think it should be possible to combine these. While both extensions deal with system properties and overlap in resetting the properties that @ClearSystemProperty and @SetSystemProperty change, they are otherwise orthogonal. These two annotations are concerned with what system properties are read, @RestoreSystemProperties is concerned with what properties are written.
  • Not sure whether we'd need to create deep copies. Technically, the system property didn't change and hence doesn't need to be restored when the value it refers to gets mutated.
  • Yes.
  • It should probably behave just like @ClearSystemProperty and @SetSystemProperty do after Enclosing classes vs Before/AfterAll #479 is solved.

@eeverman
Copy link
Contributor

I'm interested in taking a pass at this one if no one else has.

I read through your contribution guide - It is excellent! I may have to copy some of those ideas back to my project...

@nipafx
Copy link
Member

nipafx commented Nov 30, 2022

Welcome @eeverman and thank you! 😊 Looking forward to your contribution. I'll assign you this ticket and mark it as in progress. Let us know if you need any help. Feel free to open a WIP PR for that if you want to share some code.

beatngu13 pushed a commit that referenced this issue Jun 14, 2023
…es (#574 / #700)

To allow users to modify environment variables and system properties
directly in their test code, outside of the existing clear and set
annotations, this PR  introduces `RestoreEnvironmentVariables` and
`RestoreSystemProperties` to revert these resources to their pre-test state.

Closes: #574
PR: #700
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants