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

Issue/574 sys props restore v2 #700

Merged
merged 67 commits into from Jun 14, 2023

Conversation

eeverman
Copy link
Contributor

@eeverman eeverman commented Dec 7, 2022

Add restore functionality to SystemProps & EnvironmentVars Extensions (#574 / #700)

To allow users to modify System.Properties and Env Vars directly in their test code outside of the existing
set and clear annotations, new restore annotations revert these resources to their pre-test state.
The new restore annotations can be used at the class or method level and the functionality was
added to the existing extension classes.

Closes #574
PR: #700


PR checklist

The following checklist shall help the PR's author, the reviewers and maintainers to ensure the quality of this project.
It is based on our contributors guidelines, especially the "writing code" section.
It shall help to check for completion of the listed points.
If a point does not apply to the given PR's changes, the corresponding entry can be simply marked as done.

Documentation (general)

  • There is documentation (Javadoc and site documentation; added or updated)
  • There is implementation information to describe why a non-obvious source code / solution got implemented
  • Site documentation has its own .adoc file in the docs folder, e.g. docs/report-entries.adoc
  • Site documentation in .adoc file references demo in src/demo/java instead of containing code blocks as text
  • Only one sentence per line (especially in .adoc files)
  • Javadoc uses formal style, while sites documentation may use informal style

Documentation (new extension)

  • The docs/docs-nav.yml navigation has an entry for the new extension
  • The package-info.java contains information about the new extension

Code

  • Code adheres to code style, naming conventions etc.
  • Successful tests cover all changes
  • There are checks which validate correct / false usage / configuration of a functionality and there are tests to verify those checks
  • Tests use AssertJ or our own PioneerAssert (which are based on AssertJ)

Contributing

  • A prepared commit message exists
  • The list of contributions inside README.md mentions the new contribution (real name optional)

@Bukama
Copy link
Member

Bukama commented Dec 7, 2022

Hey 👋
as long is this is a draft/WIP please convert it into a draft-PR to make this clear to everyone - thank you :)

@eeverman eeverman changed the title Issue/574 sys props restore v2 WIP Issue/574 sys props restore v2 Dec 7, 2022
@eeverman eeverman marked this pull request as draft December 7, 2022 16:23
@eeverman
Copy link
Contributor Author

eeverman commented Dec 7, 2022

Drafted.

@eeverman eeverman changed the title WIP Issue/574 sys props restore v2 Issue/574 sys props restore v2 Jan 4, 2023
@eeverman eeverman marked this pull request as ready for review January 4, 2023 06:13
@eeverman
Copy link
Contributor Author

eeverman commented Jan 4, 2023

This MR is ready to go other than I am getting one spotlessJavaCheck error in the EnvironmentVariablesExtensionDemo.java file. I don't see what it's complaining about - any suggestions would be helpful.

Also, I didn't see a Gradle way to generate the user docs from the .adoc files - Is there a way to do that?

@eeverman eeverman requested review from beatngu13 and Michael1993 and removed request for Michael1993 and beatngu13 March 8, 2023 03:50
@eeverman
Copy link
Contributor Author

eeverman commented Mar 8, 2023

I think I have addressed all requested changes.
@beatngu13 and @Michael1993 - Your reviews still show 'requested changes'. I'm not sure if that is due to an unresolved issue or if it is just a manual flag that you need to flip. Either way, there are a lot of changes and a lot of conversation, so it is probably worth taking a look again.

Of all the items in the reviews, the one that stands out as perhaps only partly resolved is if the RestoreEnvironmentVariablesTests and RestoreSystemPropertiesTests test classes are really needed. I marked that issue as resolved just to cross things off the list, but it may be worth looking into. The case for keeping those tests is that they provide testing of the restore functionality outside of inner classes, which is the most common user use case. Perhaps they could be implemented better. I am frankly am out of time on this - I'm willing to delete the tests, but I don't have the bandwidth to re-write them in some other fashion. They have some none-zero value, but not as much as other tests.

@beatngu13 beatngu13 self-assigned this Mar 10, 2023
@Michael1993 Michael1993 added the full-build Triggers full build suite on PR label Jun 3, 2023
Copy link
Member

@beatngu13 beatngu13 left a comment

Choose a reason for hiding this comment

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

Once again: sorry it took so long. We can now merge this PR – thanks for the hard work! 🎉

@beatngu13 beatngu13 merged commit 15013fc into junit-pioneer:main Jun 14, 2023
21 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
full-build Triggers full build suite on PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add extension / annotation for restoring system properties
5 participants