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

Create new temporary directory/resource extension #491

Merged
merged 155 commits into from Nov 14, 2022

Conversation

jbduncan
Copy link
Contributor

@jbduncan jbduncan commented May 29, 2021

Proposed commit message:

${action} (${issues} / ${pull-request}) [max 70 characters]

${body} [max 70 characters per line]

${references}: ${issues}
PR: ${pull-request}

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
  • 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)

I hereby agree to the terms of the JUnit Pioneer Contributor License Agreement.

Copy link
Member

@nipafx nipafx left a comment

Choose a reason for hiding this comment

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

This looks good and is definitely going into the right direction. Keep at it. 😉 (And don't hesitate to let us know if you don't have the time or interest for it any more. We don't want to put pressure on you completing this.)

IssueNavigationLink {
option(name: "issueRegexp", value: '#(\\d+)')
option(name: "linkRegexp", value: 'https://github.com/junit-pioneer/junit-pioneer/issues/$1')
if (xmlFile.exists() && xmlFile.isFile()) {
Copy link
Member

Choose a reason for hiding this comment

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

You added this check, right? Can you let us know why it was necessary? If we need to change this file, we should so it in a separate change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did indeed. It was necessary because when I imported this project into my IntelliJ IDEA, the file .idea/vcs.xml was absent for some bizarre reason. So whenever I tried to run any of the Gradle tasks, it threw an exception upon trying to parse the file.

That's a good shout, putting this in a separate change. Given I'm busy IRL, is this something that someone from the core team would be able to handle? If not, then that's fine, as I've just put it in my TODO list.


@Override
public Path get() throws Exception {
return (tempDir = Files.createTempDirectory(prefix));
Copy link
Member

Choose a reason for hiding this comment

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

Doesn't this reassign a new tempDir every time get() is called? Generally speaking, I'd prefer to have a final field, whose value gets assigned on creation.

Copy link
Contributor Author

@jbduncan jbduncan Aug 1, 2021

Choose a reason for hiding this comment

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

It does. I assumed that Resource::get will be called at most once by the extension and will never be called by user code.

I drove out this code in a "minimalist" TDD fashion, without thinking much, so thanks for pointing out the flaw! It's now in the TODO list.

Comment on lines 31 to 32
// TODO: Check that the parameter is annotated with at least @New or @Shared.
// TODO: Check that the parameter is not annotated with both @New and @Shared.
Copy link
Member

Choose a reason for hiding this comment

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

This was already checked in supportsParameter. We still need to find and interpret the annotations, but we don't need to verify proper use of them.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, good shout! I agree. They're now in the TODO list to remove.

src/main/java/org/junitpioneer/jupiter/New.java Outdated Show resolved Hide resolved
ReflectionSupport.newInstance(newResourceAnnotation.value(), (Object[]) newResourceAnnotation.arguments());
extensionContext
.getStore(
ExtensionContext.Namespace.create(ResourceManagerExtension.class, extensionContext.getUniqueId()))
Copy link
Member

Choose a reason for hiding this comment

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

I don't think we need extensionContext.getUniqueId() here - each extension context already has its own store.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, okay. I assume something like Namespace.create(ResourceManagerExtension.class) in a constant like NAMESPACE will do?

extensionContext
.getStore(
ExtensionContext.Namespace.create(ResourceManagerExtension.class, extensionContext.getUniqueId()))
.put(System.identityHashCode(newResourceAnnotation) + "-resource-factory",
Copy link
Member

Choose a reason for hiding this comment

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

I understand the need for identityHashCode as follows:

  • the store is a key-value store, so we need a key
  • we need to make sure not to accidentally reuse the same key or already-registered resources would get kicked out again

Is that correct? If so, I see one other options besides identityHashCode:

  • use a random long - chances of collisions are minuscule

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's correct!

I actually made the same observation a few days ago, as I learned that System::identityHashCode isn't generally a good way of IDing things because when an object is garbage-collected, its memory location can be reused by a new object.

This may not apply in this case, but I think a long or AtomicLong would be best in case my assumption is ever invalidated.

It's already in the TODO list.

extensionContext
.getStore(
ExtensionContext.Namespace.create(ResourceManagerExtension.class, extensionContext.getUniqueId()))
.put(System.identityHashCode(newResourceAnnotation) + "-resource",
Copy link
Member

Choose a reason for hiding this comment

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

As above, but one more option:

  • ask the resource for a unique name - by default it could return its own identityHashCode (or the random long), but certain resources may already be unique (e.g. file path for Resource<Path>)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think using a random long/AtomicLong is easiest to understand, so unless you have any objections, then that's the method I will try first. :)

.orElse("unknown method")));
});
ResourceFactory<?> resourceFactory =
ReflectionSupport.newInstance(newResourceAnnotation.value(), (Object[]) newResourceAnnotation.arguments());
Copy link
Member

Choose a reason for hiding this comment

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

I know I said to pass these arguments to the constructor of the resource factory, but now I think that was wrong. I think it should be passed to ResourceFactory::create. That seems to make more sense.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, yep, that makes sense. It may make Resource(Factory) easier to understand. It's in the TODO list now.

Comment on lines 38 to 57
return Optional.of(testClass);
return Optional.ofNullable(testClass);
}

@Override
public Optional<Method> getTestMethod() {
return Optional.of(testMethod);
return Optional.ofNullable(testMethod);
Copy link
Member

Choose a reason for hiding this comment

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

Can you explain why these changes are needed?

Copy link
Contributor Author

@jbduncan jbduncan Aug 2, 2021

Choose a reason for hiding this comment

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

@nipafx It's so that I can make a TestExtensionContext that has no test method.

This is so that I can test an (admittedly extremely unlikely) edge case in ResourceManagerExtension where all hell has broken loose and:

  1. ResourceManagerExtension::supportsParameter thinks a parameter has @New, but ::resolveParameter disagrees.
  2. And JUnit 5 itself gives ::resolveParameter an ExtensionContext with an absent getTestMethod(), the contents of which I'm using as part of an exception message.

My thinking was to ensure a helpful exception is thrown even in this case.

But I can see now that the change to TestExtensionContext::getTestClass is not needed. And regardless, this generally feels convoluted.

So I'm happy to revert this hunk and avoid testing this edge case if you'd like?

(For context, here's the edge case I just talked about.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've just pushed a commit that reverts this change and the test for the edge case, as it's so incredibly unlikely that such a convoluted test isn't worth the effort of understanding it.


public @interface Shared {

Class<? extends ResourceFactory<?>> value();
Copy link
Member

Choose a reason for hiding this comment

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

I think using value as a name for the attribute makes the most sense when you can expect that it will be the only configured attribute in most use cases (because you can spare the value = ). But since we always need the name as well, users would always have to write value, in which case I think we should use a more explicit name. What about factory?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, that makes sense! It's now in the TODO list.

@jbduncan
Copy link
Contributor Author

@nipafx Wow, thank you for the detailed review! I'm still very interested in working on this, I'm just held back by IRL things again and I don't know when I'll be next free. I might be able to find time over the next few days to respond to the review.

@Bukama
Copy link
Member

Bukama commented Aug 1, 2021

@jbduncan Take your time - RL is more important than Pioneer!

jbduncan added a commit to jbduncan/junit-pioneer that referenced this pull request Oct 8, 2021
Whilst working on junit-pioneer#491, I saw that buildSrc/build.gradle was
failing if the .idea directory existed but .idea/vcs.xml did not,
which is what was happened for me with my version of IntelliJ IDEA.

This commit makes the build script more lenient by only trying to
append issue-related XML tags to .idea/vcs.xml if it exists,
rather than relying on the presence of just the .idea directory.
@jbduncan jbduncan mentioned this pull request Oct 8, 2021
13 tasks
jbduncan added a commit to jbduncan/junit-pioneer that referenced this pull request Oct 8, 2021
Whilst working on junit-pioneer#491, I saw that buildSrc/build.gradle was
failing if the .idea directory existed but .idea/vcs.xml did not,
which is what was happened for me with my version of IntelliJ IDEA.

This commit makes the build script more lenient by only trying to
append issue-related XML tags to .idea/vcs.xml if it exists,
rather than relying on the presence of just the .idea directory.

junit-pioneer#491 (comment)
PR: junit-pioneer#532
@jbduncan
Copy link
Contributor Author

jbduncan commented Oct 8, 2021

FYI, I've rebased all the commits in this PR on top of main as preparation to meta-annotate @New and @Shared with the @Resources annotation. This should make tests using @New/@Shared less verbose.

Michael1993 pushed a commit that referenced this pull request Oct 12, 2021
While working on #491, I saw that buildSrc/build.gradle was
failing if the .idea directory existed but .idea/vcs.xml did not,
which is what was happened for me with my version of IntelliJ IDEA.

This commit makes the build script more lenient by only trying to
append issue-related XML tags to .idea/vcs.xml if it exists,
rather than relying on the presence of just the .idea directory.

Related discussion: #491
PR: #532
jbduncan added a commit to jbduncan/junit-pioneer that referenced this pull request Oct 15, 2021
@Bukama
Copy link
Member

Bukama commented Dec 4, 2021

Can you give us a short summary of the current status of the draft?

@jbduncan
Copy link
Contributor Author

jbduncan commented Dec 4, 2021

@Bukama The only things left to do are as follows:

  • Ensure that @BeforeEach/@BeforeAll/@AfterEach/@AfterAll methods annotated with @Shared are run sequentially.
  • Raise some new issues to:
    • Ensure that the TemporaryDirectory resource factory implementation fixes all problems with JUnit 5's own TempDir, as described in the issue OP.
    • Create a TemporaryFile resource factory.
    • Consider replacing the concurrency tests in this PR with OpenJDK JCStress or kotlinx-lincheck.
  • Write the documentation.

I intend to remove the draft status and open this PR for review when the first point is done.

@nipafx
Copy link
Member

nipafx commented Dec 9, 2021

@jbduncan No pressure, but #141/#541 is blocked by this. 😁 If you currently don't have the time to work on this, lets discuss how you can hand this over (IIRC, there are a bunch of changes on your machine that are not in the PR, yet).

@jbduncan
Copy link
Contributor Author

jbduncan commented Dec 9, 2021

@nipafx Thanks for following up on this! I believe all the changes we worked on together are now on GitHub.

I saw #141 in my inbox recently, and I agree it looks it would benefit from my work. So I'm spending a bit of time this evening to fix the last few bits I need to do before I can put this PR forward for review.

Be on the lookout for further news. 😄

@jbduncan jbduncan marked this pull request as ready for review December 9, 2021 22:26
@jbduncan
Copy link
Contributor Author

jbduncan commented Dec 9, 2021

@nipafx @Bukama This PR is finally ready for review! 🎉

Let me know what you think.

@jbduncan
Copy link
Contributor Author

jbduncan commented Nov 8, 2022

I'm ready for the next round of reviews! (Hopefully the final one this time, fingers crossed. 🤞)

Copy link
Member

@nipafx nipafx left a comment

Choose a reason for hiding this comment

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

Found two new TODOs.

@nipafx
Copy link
Member

nipafx commented Nov 9, 2022

One more TODO: All new annotations need a @since attribute in their Javadoc. That should be the last thing we do to make sure we pick the right version.

@jbduncan
Copy link
Contributor Author

Ah, it's just occurred to me that by simplifying the intercept* logic for lifecycle methods like @BeforeEach and @BeforeAll, if multiple parallel tests try to mutate a shared resource injected into e.g. a @BeforeAll method, then race conditions could happen.

However, this would only ever be a problem if users tried to save shared resources in fields. So as another TODO for myself, I'll add a paragraph to the docs to warn users against this.

As a longer term solution, I'll raise an issue later to consider running all tests in a test class sequentially if that class has a lifecycle method injected with a shared resource.

@jbduncan
Copy link
Contributor Author

A warning about not saving resources in fields has just been added in commit 25a412f.

@jbduncan
Copy link
Contributor Author

Would you like me to fix the merge conflict between this branch and the main branch, team?

@jbduncan
Copy link
Contributor Author

I've also raised a new issue for the parallelism problem I mentioned in #491 (comment).

@jbduncan
Copy link
Contributor Author

@nipafx I've just addressed your comment about the missing @since attributes in commit ec99827. I've also fleshed out the JavaDocs a bit in that commit as well as commit e9ae084.

@nipafx nipafx dismissed Michael1993’s stale review November 14, 2022 11:24

Everything has been addressed

@jbduncan
Copy link
Contributor Author

We're so close, I can almost taste it! ✨

@nipafx nipafx merged commit 7f56ffe into junit-pioneer:main Nov 14, 2022
@nipafx
Copy link
Member

nipafx commented Nov 14, 2022

It happened! 🤯

@TWiStErRob
Copy link
Contributor

Whaaaat? 1.5 years in the making 👏🏻
image

@nipafx
Copy link
Member

nipafx commented Nov 14, 2022

Thank you so much for your infinite patience, @jbduncan! 🙏🏾 We finally made it.

@jbduncan
Copy link
Contributor Author

@nipafx You're very welcome, and thank you and everyone else for all your guidance! What a ride and learning experience it's been. 😊

@jbduncan
Copy link
Contributor Author

I'll keep an eye out for future bug reports and suggestions with this extension, so that if I have the time, I can address then as soon as possible.

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.

Create new temporary directory/resource extension
5 participants