Found 1 DeveloperFixed Pinot Test

[LucasEby]

One of the previously discovered ID tests in Apache Pinot was fixed by the developers. I was able to track down the commits from the last time it failed and when it was fixed. I have included instructions below to reproduce my results as well as the locations of the output files on the VM as proof of the results.

Output File Locations on VM

Failing output: 014:~/pinotDeveloperFixed/a02253d671d7781acc16b81628e51bd11a3005a0Output.txt

Passing output: 014:~/pinotDeveloperFixed/782b6979360b6ec28c869bbf8b7ea59e1548a3efOutput.txt

Instructions to reproduce the results

How to Reproduce the final FAILING ID Test

1. Checkout the SHA of the last FAILED commit:
   git checkout a02253d671d7781acc16b81628e51bd11a3005a0

2. Delete this line from the pom.xml:
   -javaagent:${settings.localRepository}/org/mockito/mockito-core/${mockito-core.version}/mockito-core-${mockito-core.version}.jar

3. Compile it:
   ./mvnw clean install -DskipTests -Pbin-dist -Pbuild-shaded-jar -Dlicense.skip -Drat.skip -Dcheckstyle.skip

4. Run the ID test with nondex. We are expecting nondex to FAIL after running the below command. Less than 10 iterations was found to be sufficient to generate the error, so we ran 100 just to be on the safe side.
   mvn -pl pinot-query-planner -Dtest=org.apache.pinot.query.queries.ResourceBasedQueryPlansTest#testQueryExplainPlansWithExceptions -DnondexRuns=100 -DreuseForks=false edu.illinois:nondex-maven-plugin:2.2.1:nondex -Dcheckstyle.skip=true -DnondexRunsWithoutShuffling=0

How to Reproduce Fixed ID Test:

1. Checkout commit that FIXED the ID Test:
   git checkout 782b6979360b6ec28c869bbf8b7ea59e1548a3ef

2. Delete this line from the pom.xml:
   -javaagent:${settings.localRepository}/org/mockito/mockito-core/${mockito-core.version}/mockito-core-${mockito-core.version}.jar

3. Compile it:
   ./mvnw clean install -DskipTests -Pbin-dist -Pbuild-shaded-jar -Dlicense.skip -Drat.skip -Dcheckstyle.skip

4. Run the ID test with nondex. We are expecting nondex to PASS after running the below command. Less than 10 iterations was found to be sufficient to generate the error, so we ran 100 just to be on the safe side to ensure that it was fixed.
   mvn -pl pinot-query-planner -Dtest=org.apache.pinot.query.queries.ResourceBasedQueryPlansTest#testQueryExplainPlansWithExceptions -DnondexRuns=100 -DreuseForks=false edu.illinois:nondex-maven-plugin:2.2.1:nondex -Dcheckstyle.skip=true -DnondexRunsWithoutShuffling=0

[darko-marinov]

Great finding. What was fails 1/38 cases; why delete it?

[LucasEby]

That was something someone placed there previously when they found the ID test. I inserted it back in. Thank you for catching the mistake

[darko-marinov]

As part of "Git(Hub) archaeology", can you fin who left the note and why?

For the failing check, the piece of information in Notes should be separated by ; not ,.

[LucasEby]

I fixed the failing check and updated the note so that it is more informative.

From looking at the original PR and the test itself this is what I've gathered:

• The test itself doesn't contain any loops and performs two assertions so the testCases aren't referrring to the test itself. The test utilizes a DataProvider which has multiple for loops inside of it which iterate over testCases (which are provided to the test). I believe the comment is referring to 1/38 of these testCases from the DataProvider failing.
• Originally the student had each of these test cases annotated in pr-data.csv as individual lines but they were asked to condense them into fewer lines because each of these testCases are covered inside of the same test

[darko-marinov]

How did you find the original PR? :) The summary is good. Just change that last , to ;, and I can accept your PR.

[darko-marinov]

Thanks for debugging the format checker. It has evolved over time, mostly in response by specific cases. While some changes to the format checker were made by the TAs, many were made by the students. If you want, you can find the exact part of the Python code that does this check and then change it, at least to make the error message more informative if not to change the check itself.

[darko-marinov]

Git bisect gave me the PR that was associated with the commit

Two questions here:

1. How would Git bisect give the PR (it gives just the commit)?

2. Are we talking about the same PR/commit? I was talking about Detected New Flaky Test in Pinot-query-planner #1085, which I think you found in another way, not using Git bisect.

[LucasEby]

I apologize, I ran a log command which I think returned the PR with the Pinot commit. Your question was how I found the Idoft PR though. For that I did this:

My immediate reaction was: use the pull request search and search for closed PR's which contained keywords such as "pinot" or "pinot-query-planner" to see how many were returned since I was already on the website. I found it right away when I did this.

My backup plan was to do this (just checked and it worked to):
git bisect (with bad commit being HEAD and good being HEAD1000 with the assumption the text isn't present in HEAD1000)
rg "fails 1/38" pr-data.csv (To search to see if the text was added to the file. If so, it's marked as bad, otherwise marked as good)
gh pr list --repo TestingResearchIllinois/idoft --search 11e8a81230902bfa4542e0ea299f7aac9a6d9598 --state merged

I think the smartest way to do it is probably this though:
git log -p -G "fails 1/38" -- pr-data.csv
gh pr list --repo TestingResearchIllinois/idoft --search 11e8a81230902bfa4542e0ea299f7aac9a6d9598 --state merged

I'll look into the format checker and see if there's anything I can do to improve it for this edge case and get back to you.

[LucasEby]

Created this PR to address the problem: #1908. This PR's checks should pass after the other PR is accepted and the checks are re-run.

[darko-marinov]

Thanks for clarifications.

use the pull request search and search for closed PR's which contained keywords such as "pinot" or "pinot-query-planner"

Makes perfect sense as an ad-hoc search for this case, although not as general as the other approach you describe.

gh pr list ...

Fair enough, although it's not a Git command but GitHub CLI.

As for finding the commit with fails 1/38 (before mapping it to a PR), one could also use git blame pr-data.csv | grep 1/38.

Last but not least, let's wait for Yiming to be back before accepting the other PR and then this one.