Add FilePosition in FileSelector and ClasspathResourceSelector #2253

timtebeek · 2020-04-10T15:26:40Z

Overview

Fixes #2146.

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

Definition of Done

There are no TODOs left in the code
Method preconditions are checked and documented in the method's Javadoc
Coding conventions (e.g. for logging) have been followed
Change is covered by automated tests including corner cases, errors, and exception handling
Public API has Javadoc and @API annotations
Change is documented in the User Guide and Release Notes
All continuous integration builds pass

Issue: #2146

junit-platform-engine/src/main/java/org/junit/platform/engine/discovery/FilePosition.java

platform-tests/src/test/java/org/junit/platform/engine/discovery/FilePositionTests.java

codecov · 2020-04-10T15:41:27Z

Codecov Report

Merging #2253 into master will decrease coverage by 0.04%.
The diff coverage is 82.35%.

@@             Coverage Diff              @@
##             master    #2253      +/-   ##
============================================
- Coverage     90.41%   90.36%   -0.05%     
- Complexity     4479     4512      +33     
============================================
  Files           391      392       +1     
  Lines         10887    10943      +56     
  Branches        875      891      +16     
============================================
+ Hits           9843     9889      +46     
- Misses          780      785       +5     
- Partials        264      269       +5

Impacted Files	Coverage Δ	Complexity Δ
.../junit/platform/engine/discovery/FileSelector.java	`68.75% <66.66%> (-2.68%)`	`9.00 <3.00> (+2.00)`	⬇️
...rm/engine/discovery/ClasspathResourceSelector.java	`70.58% <75.00%> (+1.35%)`	`9.00 <4.00> (+3.00)`
.../junit/platform/engine/discovery/FilePosition.java	`82.97% <82.97%> (ø)`	`25.00 <25.00> (?)`
.../platform/engine/discovery/DiscoverySelectors.java	`89.79% <100.00%> (+0.32%)`	`34.00 <4.00> (+3.00)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a9e8a61...2fbc96d. Read the comment docs.

junit-platform-engine/src/main/java/org/junit/platform/engine/discovery/FilePosition.java

...platform-engine/src/main/java/org/junit/platform/engine/support/descriptor/FilePosition.java

...form-engine/src/main/java/org/junit/platform/engine/discovery/ClasspathResourceSelector.java

marcphilipp

Looks good except for a few details! Most importantly, we should decide what the factory methods in DiscoverySelectors should look like.

junit-platform-engine/src/main/java/org/junit/platform/engine/discovery/DiscoverySelectors.java

...platform-engine/src/main/java/org/junit/platform/engine/support/descriptor/FilePosition.java

junit-platform-engine/src/main/java/org/junit/platform/engine/discovery/FilePosition.java

platform-tests/src/test/java/org/junit/platform/engine/discovery/FilePositionTests.java

...platform-engine/src/main/java/org/junit/platform/engine/support/descriptor/FilePosition.java

timtebeek · 2020-04-10T19:17:44Z

Ok I've hopefully addressed all of your suggestions as intended; I hope it's ok I marked them as "resolved"; If I shouldn't have done that please let me know!

~~The only outstanding issue (I think) is this slight gaffe on my part where I need some direction: https://github.com/junit-team/junit5/pull/2253/files#r406852337~~

Since resolved in decb669

...gine/src/main/java/org/junit/platform/engine/support/descriptor/ClasspathResourceSource.java

timtebeek · 2020-04-29T09:39:15Z

Just noticed 1978513 in master; with this PR added to the 5.7 M2 milestone, should I merge in master and add a rough outline for the changes contained in this PR?

I know this isn't a high priority new feature or fix, but would nonetheless be nice to see this finished if possible, before any other branches introduce conflicts. :)

marcphilipp · 2020-05-01T14:36:07Z

@timtebeek Sorry for the delay. We discussed it briefly in the last team call. The current solution still contains breaking changes so we were a bit torn whether to just duplicate the FilePosition class to avoid the messy (not your fault!) deprecation work and the confusion caused by it. Would you mind changing this PR so the FilePosition class and its tests are copied and the deprecation is reverted? Sorry for the back ans forth! As for the release notes: yes, please rebase on master and add a bullet point.

timtebeek · 2020-05-02T10:51:42Z

@marcphilipp No problem at all; I've reverted the deprecation changes, created a clean copy of FilePosition and updated the release notes. Could you have another look? :)

timtebeek · 2020-05-12T20:07:51Z

@marcphilipp any update on this issue? Just trying to keep this flowing through; if there's any changes I have to make still let me know!

juliette-derancourt

Hey @timtebeek, left you a few comments/suggestions on the PR! :)

platform-tests/src/test/java/org/junit/platform/engine/discovery/FilePositionTests.java

junit-platform-engine/src/main/java/org/junit/platform/engine/discovery/DiscoverySelectors.java

documentation/src/docs/asciidoc/release-notes/release-notes-5.7.0-M2.adoc

junit-platform-engine/src/main/java/org/junit/platform/engine/discovery/DiscoverySelectors.java

...form-engine/src/main/java/org/junit/platform/engine/discovery/ClasspathResourceSelector.java

junit-platform-engine/src/main/java/org/junit/platform/engine/discovery/FileSelector.java

platform-tests/src/test/java/org/junit/platform/engine/discovery/DiscoverySelectorsTests.java

Co-authored-by: Juliette de Rancourt <derancourt.juliette@gmail.com>

timtebeek · 2020-05-16T09:12:10Z

@juliette-derancourt Thanks a lot for the detailed feedback; I've applied all your suggestions. Let me know if there's anything else! :)

platform-tests/src/test/java/org/junit/platform/engine/discovery/DiscoverySelectorsTests.java

…ry/DiscoverySelectorsTests.java

juliette-derancourt

Thank you for making the changes @timtebeek, looks good to me! 👍

juliette-derancourt · 2020-05-16T14:13:52Z

junit-platform-engine/src/main/java/org/junit/platform/engine/discovery/FilePosition.java

+ * @since 1.7
+ */
+@API(status = STABLE, since = "1.7")
+public class FilePosition implements Serializable {


@junit-team/junit-lambda Should we mention in the Javadoc that this class is actually a copy of descriptor.FilePosition?

juliette-derancourt · 2020-05-22T21:06:38Z

Thanks for the PR @timtebeek!

Related to #2253

Tim te Beek added 5 commits April 5, 2020 13:34

Support FilePosition in FileSelector & ClasspathResourceSelector

49b8684

Issue: #2146

Add JavaDoc

a65fb00

Add constructor unit tests

6d8fe26

Move, duplicate & deprecate FilePosition as suggested

612df8e

Move FilePositionTests and have it extend AbstractEqualsAndHashCodeTests

71130a6

timtebeek commented Apr 10, 2020

View reviewed changes

junit-platform-engine/src/main/java/org/junit/platform/engine/discovery/FilePosition.java Outdated Show resolved Hide resolved

timtebeek commented Apr 10, 2020

View reviewed changes

platform-tests/src/test/java/org/junit/platform/engine/discovery/FilePositionTests.java Outdated Show resolved Hide resolved

Add FileSelector DiscoverySelectors with arguments for line and column

2c5ece1

Cover selectFile and selectClasspathResource with file position

95557dd

timtebeek commented Apr 10, 2020

View reviewed changes

junit-platform-engine/src/main/java/org/junit/platform/engine/discovery/FilePosition.java Outdated Show resolved Hide resolved

Add position in toString

1202335

timtebeek mentioned this pull request Apr 10, 2020

Support FilePosition in discovery selectors #2146

Closed

Apply spotless

98a361a

mpkorstanje reviewed Apr 10, 2020

View reviewed changes

...platform-engine/src/main/java/org/junit/platform/engine/support/descriptor/FilePosition.java Outdated Show resolved Hide resolved

mpkorstanje reviewed Apr 10, 2020

View reviewed changes

...platform-engine/src/main/java/org/junit/platform/engine/support/descriptor/FilePosition.java Outdated Show resolved Hide resolved

mpkorstanje reviewed Apr 10, 2020

View reviewed changes

...platform-engine/src/main/java/org/junit/platform/engine/support/descriptor/FilePosition.java Outdated Show resolved Hide resolved

mpkorstanje reviewed Apr 10, 2020

View reviewed changes

...form-engine/src/main/java/org/junit/platform/engine/discovery/ClasspathResourceSelector.java Outdated Show resolved Hide resolved

marcphilipp requested changes Apr 10, 2020

View reviewed changes

Tim te Beek added 4 commits April 10, 2020 20:54

Address review comments regarding FilePosition classes

f8b14c1

Optional return types for nullable position

092ff61

Take FilePosition as argument in DiscoverySelectors as suggested

2520537

Adapt tests to use FilePosition arguments and Optional return types

fb9cec7

Adopt public static Optional<? extends FilePosition> fromQuery in parent

decb669

timtebeek requested a review from marcphilipp April 10, 2020 20:15

marcphilipp requested changes Apr 11, 2020

View reviewed changes

...gine/src/main/java/org/junit/platform/engine/support/descriptor/ClasspathResourceSource.java Outdated Show resolved Hide resolved

Tim te Beek added 3 commits April 11, 2020 12:52

Add alternative getFilePosition() and restore getPosition() return type

a768cf4

Deprecate FileSource/ClasspathResourceSource getPosition() methods

dd3d8f7

Also restore original arguments to Source from methods

f80fcb9

marcphilipp added the status: team discussion label Apr 19, 2020

marcphilipp approved these changes Apr 19, 2020

View reviewed changes

marcphilipp removed the status: team discussion label May 1, 2020

Tim te Beek added 5 commits May 2, 2020 12:12

Merge branch 'master' of github.com:junit-team/junit5

528ea87

Revert changes to org.junit.platform.engine.support.descriptor package

0a14c1b

Create a copy of FilePosition without further modifier/generics changes

5baa390

Make FilePosition fields in Selectors final

e996f17

Update release notes

28cbaea

timtebeek requested a review from marcphilipp May 2, 2020 10:50

Merge branch 'master' into master

296cddb

marcphilipp requested review from juliette-derancourt and removed request for marcphilipp May 15, 2020 10:43

juliette-derancourt reviewed May 16, 2020

View reviewed changes

Tim te Beek and others added 4 commits May 16, 2020 09:15

Apply suggestions from code review

267ef71

Co-authored-by: Juliette de Rancourt <derancourt.juliette@gmail.com>

Delegate selectFile/selectClasspath methods to copy with FilePosition

40bc12a

Remove constructors without FilePosition; Update tests to match

50ba225

Separate tests for DiscoverySelectors with FilePosition argument

1910558

timtebeek commented May 16, 2020

View reviewed changes

platform-tests/src/test/java/org/junit/platform/engine/discovery/DiscoverySelectorsTests.java Outdated Show resolved Hide resolved

Update platform-tests/src/test/java/org/junit/platform/engine/discove…

2fbc96d

…ry/DiscoverySelectorsTests.java

timtebeek requested a review from juliette-derancourt May 16, 2020 18:19

juliette-derancourt approved these changes May 16, 2020

View reviewed changes

juliette-derancourt changed the title ~~Add FilePosition in FileSelector and ClasspathResourceSelector for #2146~~ Add FilePosition in FileSelector and ClasspathResourceSelector May 16, 2020

juliette-derancourt merged commit 375792d into junit-team:master May 22, 2020

juliette-derancourt added a commit that referenced this pull request May 22, 2020

Add note in Javadoc about the duplication of FilePosition

2df0b93

Related to #2253

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add FilePosition in FileSelector and ClasspathResourceSelector #2253

Add FilePosition in FileSelector and ClasspathResourceSelector #2253

timtebeek commented Apr 10, 2020 •

edited by juliette-derancourt

Loading

codecov bot commented Apr 10, 2020 •

edited

Loading

marcphilipp left a comment

timtebeek commented Apr 10, 2020 •

edited

Loading

timtebeek commented Apr 29, 2020

marcphilipp commented May 1, 2020

timtebeek commented May 2, 2020

timtebeek commented May 12, 2020

juliette-derancourt left a comment

timtebeek commented May 16, 2020

juliette-derancourt left a comment

juliette-derancourt May 16, 2020

juliette-derancourt commented May 22, 2020

Add FilePosition in FileSelector and ClasspathResourceSelector #2253

Add FilePosition in FileSelector and ClasspathResourceSelector #2253

Conversation

timtebeek commented Apr 10, 2020 • edited by juliette-derancourt Loading

Overview

Definition of Done

codecov bot commented Apr 10, 2020 • edited Loading

Codecov Report

marcphilipp left a comment

Choose a reason for hiding this comment

timtebeek commented Apr 10, 2020 • edited Loading

timtebeek commented Apr 29, 2020

marcphilipp commented May 1, 2020

timtebeek commented May 2, 2020

timtebeek commented May 12, 2020

juliette-derancourt left a comment

Choose a reason for hiding this comment

timtebeek commented May 16, 2020

juliette-derancourt left a comment

Choose a reason for hiding this comment

juliette-derancourt May 16, 2020

Choose a reason for hiding this comment

juliette-derancourt commented May 22, 2020

timtebeek commented Apr 10, 2020 •

edited by juliette-derancourt

Loading

codecov bot commented Apr 10, 2020 •

edited

Loading

timtebeek commented Apr 10, 2020 •

edited

Loading