8285452: Add a new test library API to replace a file content using FileUtils.java #8360

sisahoo · 2022-04-22T12:16:07Z

A new API to support replacing selective lines with desired content.

Progress

Change must not contain extraneous whitespace
Commit message must refer to an issue
Change must be properly reviewed (1 reviews required, with at least 1 reviewer)

Issue

JDK-8285452: Add a new test library API to replace a file content using FileUtils.java

Reviewers

Weijun Wang (@wangweij - Reviewer)
Daniel Fuchs (@dfuch - Reviewer)

Contributors

Weijun Wang <weijun@openjdk.org>

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/8360/head:pull/8360
$ git checkout pull/8360

Update a local copy of the PR:
$ git checkout pull/8360
$ git pull https://git.openjdk.java.net/jdk pull/8360/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 8360

View PR using the GUI difftool:
$ git pr show -t 8360

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/8360.diff

…ls.java" This reverts commit 96fdbd0.

bridgekeeper · 2022-04-22T12:17:37Z

👋 Welcome back ssahoo! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

openjdk · 2022-04-22T12:20:26Z

@sisahoo The following label will be automatically applied to this pull request:

core-libs

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

mlbridge · 2022-04-22T12:23:57Z

Webrevs

test/lib/jdk/test/lib/util/FileUtils.java

sisahoo

API doc added along with some changes in the code.

sisahoo · 2022-04-22T15:53:19Z

/contributor add weijun

openjdk · 2022-04-22T15:53:43Z

@sisahoo
Contributor Weijun Wang <weijun@openjdk.org> successfully added.

dfuch · 2022-04-22T16:06:22Z

test/lib/jdk/test/lib/util/FileUtils.java

+            var froms = from.stream()
+                    .map(String::trim)
+                    .collect(Collectors.joining());
+            if (!removed.equals(froms)) {
+                throw new IOException("Removed not the same");
+            }


That's a bit strange. I would suggest to return the removed lines instead, or to pass a Consumer<String> (or even better, a Predicate<String> ?) that will accept the removed lines. You could continue to remove if the predicate returns true and throw if it returns false. It would also enable you to tell exactly which line failed the check.

We just want to provide the removed lines and the added lines at the same time (just like what a patch file looks like). The exception here can probably be enhanced to show the actual difference of removed from from. Two blocks of code (call and callback) would be needed if a consumer or predicate is used, and I don't feel it's worth doing. Here I've already trimmed the lines to make sure whitespaces do not matter.

RogerRiggs · 2022-04-26T16:54:39Z

Can you elaborate on the use case, what tests would it be used in?

The hardcoded from and to seem very rigid and require knowledge of the exact format.
That might lead to very fragile tests.

Is this equivalent to looking for a substring aka String.contains(xxx) of the input file to replace (with line ending normalization)?

wangweij · 2022-04-26T17:26:43Z

We have a test that dynamically downloads a source file from an artifact server, patches it, compiles it, and then runs it. We don't want to include a static copy of the file inside the test.

This method is just a Java version of the patch tool: the from and to mimic the - and + lines in a patch file, and fromLine and toLine mimic @@ -lineno, +lineno @@ . The from side is compared to existing lines trimmed and concatenated. We can add new lines during the concatenation but the trimming is useful so there's no need to add indentation to the argument.

Yes, all fromLine, toLine, from, and to are hardcoded, but inside a patch file each @@ -lineno, +lineno @@ and the +/- lines are also hardcoded. The difference is that a patch file have context so it can detect a small line number change. This method does not have context and the replacement must be exact. The current method can only process one "hunk" but it can be enhanced later.

It's not equivalent to a find/replace because replacement can only happen between fromLine and toLine.

sisahoo · 2022-04-27T09:22:19Z

I can provide additional Test to Test this Test library.

wangweij · 2022-04-27T13:31:01Z

Or we can provide an example in the method spec.

RogerRiggs · 2022-04-27T13:42:23Z

If it is just for one/few tests, it would fit better in the directory with the test or in the test itself.
This doesn't seem like enough of a general purpose function to be in the test library.

Overwriting the input file seems like a bad choice, it will be a trap for someone.
It prevents the use of the function in a case where the output is written to a different file.

wangweij · 2022-04-27T13:52:14Z

Currently it's just for one test, but it's the exact kind of method that should go into the test library, i.e. a general purpose file manipulation utility that can be useful by everyone.

patch overwrites the input file (by default) too. We can always enhance it to support -o.

RogerRiggs · 2022-04-27T13:58:20Z

Please change the issue/pr title to indicate it is a new API in the test library.
The problem with single purpose APIs with not enough forethought is that they are not discoverable
in the library and fall into disuse or are not appropriate for someone else to find and use.

wangweij · 2022-04-27T14:08:55Z

I've updated the title of the bug. Siba can update the PR title.

You're right that it's not easy to discover such APIs. We put it in FileUtils hoping people who does not want to write their own method will search from there first.

As for the API itself, we've imagined something like

FileUtils.patch(inputFile)
        .with(1, 10, List.of(lines), List.of(newLines))
        .with(100, 100, linesAsAString, newLinesAsAString)
        .writeTo(outputFile)

but the current form is the simplest case and could be kept even if we have a verbose one.

sisahoo · 2022-04-28T14:50:48Z

I will do a small change in method signature patch(Path path, int fromLine, int toLine, List from, String to) where the argument "List from" to be "String from". This looks more useful given java supports multi-line String. Also a new open Test will be added into this change-set to verify this new API .

…ileUtils.java

sisahoo · 2022-04-29T10:46:58Z

test/jdk/java/nio/file/Files/FileUtilsTest.java

+b
+c""";
+        // 1st line has a space character
+        String sabc = " " + System.lineSeparator() + abc;


It's strange that jcheck fails, if there is space character in beginning of line in a multiline string. So i have to follow this way add a space character in the beginning of multiline string.

You can use \s instead of space. Then you will have no complaints.

wangweij · 2022-04-29T11:53:15Z

test/lib/jdk/test/lib/util/FileUtils.java

        var lines = Files.readAllLines(path);
        if (from != null) {
            var removed = "";
            for (int i = fromLine; i <= toLine; i++) {
                removed += lines.remove(fromLine - 1).trim();
            }
-            var froms = from.stream()
+            var froms = Arrays.asList(from.split(System.lineSeparator())).stream()


How about just using from.lines()?

jaikiran · 2022-04-29T12:00:29Z

test/lib/jdk/test/lib/util/FileUtils.java

+    /**
+     * Patches a part of a file.
+     * @param path of file
+     * @param fromLine the first line to patch. This is the number you see in an editor, 1-based.


Perhaps this should mention whether the fromLine is inclusive, like it's noted for the toLine?

jaikiran · 2022-04-29T12:02:59Z

test/lib/jdk/test/lib/util/FileUtils.java

+     * @param to the newly added line, can be multiple lines or empty. Cannot be null.
+     * @throws IOException
+     */
+    public static void patch(Path path, int fromLine, int toLine, String from, String to) throws IOException {


Should this method check whether the fromLine and toLine are valid values? Things like, negative value or 0 or toLine being less than fromLine. I'm not familiar with the expectations of test library code - maybe those checks aren't necessary since this is a test util?

lines.remove() and lines.subList() will throw the correct exception. Since you asked, we can add it.

Now that we call subList at the beginning, I think there's no need to explicitly perform a check.

Hello Weijun, the change to this part of the code in context of index checks, looks fine to me now. Thank you for considering it.

dfuch · 2022-04-29T12:35:59Z

test/lib/jdk/test/lib/util/FileUtils.java

+        if (from != null) {
+            var removed = "";
+            for (int i = fromLine; i <= toLine; i++) {
+                removed += lines.remove(fromLine - 1).trim();


shouldn't you insert a "\n" ? otherwise concatenating lines "ab" and "c" will be the same as concatenating lines "a" and "bc".

Also calling trim() assumes that white spaces are not significant. This might not be the case in the general case (for instance - think of markdown files were leading spaces are significant).

The comparison is intentionally made lax so the caller does not need to provide the exact indentation or even new line characters. We think along with fromLine and toLine this is enough to make sure we are not modifying the wrong lines.

Shouldn't the comparison be better implemented by the caller then, who will know whether spaces are important or not? That's why I had suggested taking a Predicate<String> that could be called with each line removed, and the caller could interrupt the parsing by returning false when they detect a mismatch with what they expect.

We can provide a more sophisticated Function<String,String> replacer if we want to let user to customize all the details. This time we still only want them to be string literals. I agree we can keep the new lines inside, but trimming on each line and the final block is still useful so caller does not need to care about indentation and empty lines at both ends.

OK - if you keep the internal new lines I have no objection. The API doc should however say that lines will be trimmed before comparing them.

LanceAndersen · 2022-04-29T17:04:05Z

test/jdk/java/nio/file/Files/FileUtilsTest.java

+        // Replace a range of lines with mismatched lines
+        test(abcList, 1, 3, "ab", xyz, "xyz");
+    }
+


Any thought of using TestNG with a DataProvider? Seems more efficient

openjdk · 2022-05-02T13:12:16Z

@sisahoo This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8285452: Add a new test library API to replace a file content using FileUtils.java

Co-authored-by: Weijun Wang <weijun@openjdk.org>
Reviewed-by: weijun, dfuchs

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 132 new commits pushed to the master branch:

0f62cb6: 8285921: serviceability/dcmd/jvmti/AttachFailed/AttachReturnError.java fails on Alpine
6fcd322: 8279622: C2: miscompilation of map pattern as a vector reduction
af1ee1c: 8283684: IGV: speed up filter application
7a48351: 8280568: IGV: Phi inputs and pinned nodes are not scheduled correctly
64b5b2b: 8282828: CDS uncompressed oops archive is not deterministic
45ca81f: 8285915: failure_handler: gather the contents of /etc/hosts file
3420a1a: 8286013: Incorrect test configurations for compiler/stable/TestStableShort.java
fbcd874: 8285979: G1: G1SegmentedArraySegment::header_size() is incorrect since JDK-8283368
50a4df8: 8286024: PKCS12 keystore shows "DES/CBC" as the algorithm of a DES SecretKeyEntry
f973b78: 8286028: Some -Xlint keys are missing in javac man page
... and 122 more: https://git.openjdk.java.net/jdk/compare/82f0ac02e00f2c3ef3b10b150fcb7c7243a529bc...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

dfuch

LGTM

sisahoo · 2022-05-04T06:55:23Z

/integrate

openjdk · 2022-05-04T06:56:29Z

Going to push as commit 0462d5a.
Since your change was applied there have been 141 commits pushed to the master branch:

4282fb2: 8286063: check compiler queue after calling AbstractCompiler::on_empty_queue
075ce8a: 8286069: keytool prints out wrong key algorithm for -importpass command
efcd3d3: 8286088: add comment to InstallAsyncExceptionHandshake destructor
f82dd76: 8285885: Replay compilation fails with assert(is_valid()) failed: check invoke
be67acd: 8285832: runtime/Thread/TooSmallStackSize.java failed "assert(k->is_initialized()) failed: need to increase java_thread_min_stack_allowed calculation"
39e50c2: 8273506: java Robot API did the 'm' keypress and caused /awt/event/KeyEvent/KeyCharTest/KeyCharTest.html is timing out on macOS 12
3cbf769: 8285977: Add links to IEEE 754 specification
4434c7d: 8265360: several compiler/whitebox tests fail with "private compiler.whitebox.SimpleTestCaseHelper(int) must be compiled"
ffca23a: 8284490: Remove finalizer method in java.security.jgss
0f62cb6: 8285921: serviceability/dcmd/jvmti/AttachFailed/AttachReturnError.java fails on Alpine
... and 131 more: https://git.openjdk.java.net/jdk/compare/82f0ac02e00f2c3ef3b10b150fcb7c7243a529bc...master

Your commit was automatically rebased without conflicts.

openjdk · 2022-05-04T06:56:42Z

@sisahoo Pushed as commit 0462d5a.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

sisahoo added 3 commits April 22, 2022 17:37

8285452: Support new API to replace a file content in FileUtils.java

96fdbd0

Revert "8285452: Support new API to replace a file content in FileUti…

f55fb49

…ls.java" This reverts commit 96fdbd0.

8285452: Support new API to replace a file content using FileUtils.java

39fa164

openjdk bot added the rfr Pull request is ready for review label Apr 22, 2022

openjdk bot added the core-libs core-libs-dev@openjdk.org label Apr 22, 2022

Update FileUtils.java

0ec01e0

dfuch reviewed Apr 22, 2022

View reviewed changes

test/lib/jdk/test/lib/util/FileUtils.java Outdated Show resolved Hide resolved

Update FileUtils.java

ef5dc31

sisahoo commented Apr 22, 2022

View reviewed changes

dfuch reviewed Apr 22, 2022

View reviewed changes

sisahoo changed the title ~~8285452: Support new API to replace a file content using FileUtils.java~~ 8285452: Add a new test library API to replace a file content using FileUtils.java Apr 28, 2022

8285452: Add a new test library API to replace a file content using F…

1412593

…ileUtils.java

openjdk bot removed the rfr Pull request is ready for review label Apr 29, 2022

updated to add space character in begining of multiline string

e18cd8c

openjdk bot added the rfr Pull request is ready for review label Apr 29, 2022

sisahoo commented Apr 29, 2022

View reviewed changes

wangweij reviewed Apr 29, 2022

View reviewed changes

jaikiran reviewed Apr 29, 2022

View reviewed changes

sisahoo added 2 commits April 29, 2022 17:49

8285452: updated to from.lines()

da6a214

8285452: updated to use lines()

0b7dc2f

dfuch reviewed Apr 29, 2022

View reviewed changes

LanceAndersen reviewed Apr 29, 2022

View reviewed changes

sisahoo added 2 commits May 2, 2022 12:42

8285452: Tests updated

ff0f9cb

8285452: Tests updated

f73faf8

wangweij approved these changes May 2, 2022

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label May 2, 2022

dfuch approved these changes May 3, 2022

View reviewed changes

openjdk bot added the integrated Pull request has been integrated label May 4, 2022

openjdk bot closed this May 4, 2022

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels May 4, 2022

8285452: Add a new test library API to replace a file content using FileUtils.java #8360

8285452: Add a new test library API to replace a file content using FileUtils.java #8360

Conversation

sisahoo commented Apr 22, 2022 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Contributors

Reviewing

bridgekeeper bot commented Apr 22, 2022

openjdk bot commented Apr 22, 2022

mlbridge bot commented Apr 22, 2022 • edited Loading

Webrevs

sisahoo left a comment

Choose a reason for hiding this comment

sisahoo commented Apr 22, 2022

openjdk bot commented Apr 22, 2022

dfuch Apr 22, 2022 • edited Loading

Choose a reason for hiding this comment

wangweij Apr 22, 2022 • edited Loading

Choose a reason for hiding this comment

RogerRiggs commented Apr 26, 2022

wangweij commented Apr 26, 2022 • edited Loading

sisahoo commented Apr 27, 2022

wangweij commented Apr 27, 2022

RogerRiggs commented Apr 27, 2022

wangweij commented Apr 27, 2022

RogerRiggs commented Apr 27, 2022

wangweij commented Apr 27, 2022 • edited Loading

sisahoo commented Apr 28, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dfuch Apr 29, 2022 • edited Loading

Choose a reason for hiding this comment

wangweij Apr 29, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

openjdk bot commented May 2, 2022 • edited Loading

dfuch left a comment

Choose a reason for hiding this comment

sisahoo commented May 4, 2022

openjdk bot commented May 4, 2022

openjdk bot commented May 4, 2022

sisahoo commented Apr 22, 2022 •

edited by openjdk bot

Loading

mlbridge bot commented Apr 22, 2022 •

edited

Loading

dfuch Apr 22, 2022 •

edited

Loading

wangweij Apr 22, 2022 •

edited

Loading

wangweij commented Apr 26, 2022 •

edited

Loading

wangweij commented Apr 27, 2022 •

edited

Loading

sisahoo commented Apr 28, 2022 •

edited

Loading

dfuch Apr 29, 2022 •

edited

Loading

wangweij Apr 29, 2022 •

edited

Loading

openjdk bot commented May 2, 2022 •

edited

Loading