TestingResearchIllinois / idoft

34 stars 248 forks source link

Updating the status of six tests in json-io to "MovedOrRenamed" #1375

Closed Jake-WangZhi closed 1 week ago

Jake-WangZhi commented 1 week ago

Six flaky tests on json-io has been moved to different module paths. Here is the commit: https://github.com/jdereg/json-io/commit/45699f116bdaac9449cc58fbdd9310f00ac6df5b

darko-marinov commented 1 week ago

Please split PRs for Opened (which is very easy to accept) and MovedOrRenamed (which takes more inspection).

Jake-WangZhi commented 1 week ago

Please split PRs for Opened (which is very easy to accept) and MovedOrRenamed (which takes more inspection).

Okay, should I close this PR then?

darko-marinov commented 1 week ago

No, please do not close. Revise this PR to keep one of the two kinds of changes. Then open a new PR with the other kind.

Jake-WangZhi commented 1 week ago

@darko-marinov I've revised it. Thank you

darko-marinov commented 1 week ago

Please squash commits (and rebase your work on top of the latest main).

Jake-WangZhi commented 1 week ago

Please squash commits (and rebase your work on top of the latest main).

@darko-marinov Done! Let me know if there is anything else needed. Thank you

Jake-WangZhi commented 1 week ago

Please squash commits (and rebase your work on top of the latest main).

@darko-marinov Done! Let me know if there is anything else needed. Thank you

I noticed two additional flaky tests were moved in the same commit, so I added them as well

darko-marinov commented 1 week ago

Where were the files moved? What do you get for git show --name-status 45699f116bdaac9449cc58fbdd9310f00ac6df5b | grep FILE for those FILEs?

Are the tests in the new location still ID in the latest project version?

Jake-WangZhi commented 1 week ago

Where were the files moved? What do you get for git show --name-status 45699f116bdaac9449cc58fbdd9310f00ac6df5b | grep FILE for those FILEs?

Are the tests in the new location still ID in the latest project version?

@darko-marinov Five of them are still ID, one of them is not.

The maintainer just got rid of the entire util folder in this commit: https://github.com/jdereg/json-io/commit/45699f116bdaac9449cc58fbdd9310f00ac6df5b. Everything is placed under io folder now.

Jake-WangZhi commented 1 week ago

Where were the files moved? What do you get for git show --name-status 45699f116bdaac9449cc58fbdd9310f00ac6df5b | grep FILE for those FILEs?

Are the tests in the new location still ID in the latest project version?

@darko-marinov Sorry for the late reply, here is what I got for running the command you asked for each file

jakew4@fa24-cs527-043:~/json-io/src/test/java/com/cedarsoftware/io$ git show --name-status 45699f116bdaac9449cc58fbdd9310f00ac6df5b | grep AtomicBooleanTest.java
R084    src/test/java/com/cedarsoftware/util/io/AtomicBooleanTest.java  src/test/java/com/cedarsoftware/io/AtomicBooleanTest.java
jakew4@fa24-cs527-043:~/json-io/src/test/java/com/cedarsoftware/io$ git show --name-status 45699f116bdaac9449cc58fbdd9310f00ac6df5b | grep AtomicLongTest.java
R085    src/test/java/com/cedarsoftware/util/io/AtomicLongTest.java     src/test/java/com/cedarsoftware/io/AtomicLongTest.java
jakew4@fa24-cs527-043:~/json-io/src/test/java/com/cedarsoftware/io$ git show --name-status 45699f116bdaac9449cc58fbdd9310f00ac6df5b | grep CollectionTests.java
R099    src/test/java/com/cedarsoftware/util/io/CollectionTests.java    src/test/java/com/cedarsoftware/io/CollectionTests.java
jakew4@fa24-cs527-043:~/json-io/src/test/java/com/cedarsoftware/io$ git show --name-status 45699f116bdaac9449cc58fbdd9310f00ac6df5b | grep MapsTest.java
R099    src/test/java/com/cedarsoftware/util/io/MapOfMapsTest.java      src/test/java/com/cedarsoftware/io/MapOfMapsTest.java
R099    src/test/java/com/cedarsoftware/util/io/MapsTest.java   src/test/java/com/cedarsoftware/io/MapsTest.java
jakew4@fa24-cs527-043:~/json-io/src/test/java/com/cedarsoftware/io$ git show --name-status 45699f116bdaac9449cc58fbdd9310f00ac6df5b | grep NoTypeTest.java
R095    src/test/java/com/cedarsoftware/util/io/NoTypeTest.java src/test/java/com/cedarsoftware/io/NoTypeTest.java
jakew4@fa24-cs527-043:~/json-io/src/test/java/com/cedarsoftware/io$ git show --name-status 45699f116bdaac9449cc58fbdd9310f00ac6df5b | grep PrettyPrintTest.java
R098    src/test/java/com/cedarsoftware/util/io/PrettyPrintTest.java    src/test/java/com/cedarsoftware/io/PrettyPrintTest.java
darko-marinov commented 1 week ago

Okay, I'm accepting MovedOrRenamed, but you should likely add the new ID tests that you found in the latest commit.

Jake-WangZhi commented 1 week ago

Okay, I'm accepting MovedOrRenamed, but you should likely add the new ID tests that you found in the latest commit.

@darko-marinov I was doing that, but you asked me to split them, so I created another PR, which has already been merged: https://github.com/TestingResearchIllinois/idoft/pull/1378.

I fixed four of the tests and added them to iDoFT. I’ll add the other one that is still ID.

I had a question about the one that is no longer ID. Should I still add it with the updated module path and mark its status as 'deleted'?

darko-marinov commented 1 week ago

Sorry I forgot that the previously accepted PR was related to these same tests.

For the fifth test, if you can confirm (with some saved evidence) that it was ID somewhere between being moved and being deleted, then you can add a new row with Deleted. Otherwise, if you can't confirm that it was ever ID after being moved, it's not appropriate to add a new row.

Jake-WangZhi commented 1 week ago

Sorry I forgot that the previously accepted PR was related to these same tests.

For the fifth test, if you can confirm (with some saved evidence) that it was ID somewhere between being moved and being deleted, then you can add a new row with Deleted. Otherwise, if you can't confirm that it was ever ID after being moved, it's not appropriate to add a new row.

@darko-marinov Hey Dr. Marinov, thanks for the guidance, I've opened a PR for this case. Could you take a look when available? Thank you

https://github.com/TestingResearchIllinois/idoft/pull/1395