Add Windows support for testers #319
Conversation
There was a problem hiding this comment.
In addition to the concerns I raised in my comments, I have one other large concern: Specimin's CI runs in Unix, so none of these scripts are tested anywhere. One advantage of having two different sets of scripts that are intended to accomplish the same purpose is that we can test them against each other (this is technically called differential testing), and I think it would be wise for us to do that.
build.gradle
Outdated
| @@ -53,7 +53,11 @@ task requireJavadoc(type: JavaExec) { | |||
|
|
|||
| task expectedTestOutputsMustCompile(type: Exec) { | |||
| // TODO: should this task run in CI, or as part of the regular tests? | |||
| commandLine "sh", "typecheck_test_outputs.sh" | |||
| if (System.getProperty('os.name').toLowerCase().startsWith('windows')) { | |||
| commandLine "cmd", "/c", "your_batch_file.bat" | |||
There was a problem hiding this comment.
I think this should be typecheck_test_outputs.bat rather than your_batch_file.bat.
Have you tried running this task and checking to make sure it does what you expect? I would think that this problem would show up immediately if you did.
There was a problem hiding this comment.
Yes, I did, sorry. I accidentally deleted the file before I committed so I had to re-copy and paste the template code, but it looks like I forgot to change it again, thanks!
check_differences.bat
Outdated
|
|
||
| rem On Windows, instead of using the diff command for SpeciminTestExecutor, we can use this | ||
| rem command instead since diff is not included by default. This script outputs 0 if no differences | ||
| rem are found, 1 if a difference in file structure or file content (all whitespace removed) is found |
There was a problem hiding this comment.
Did you write this script yourself, or did you source it from somewhere? If the former, we need to add some tests for it (it's complex enough that I'm sure there are edge cases, and I'm not enough of a Windows batch file expert to be sure that this script is correct through inspection alone); if the latter, we should 1) give credit where credit is due, and 2) if possible, depend on wherever this script is sourced from directly, so that we'll receive bugfixes/updates if there are any.
There was a problem hiding this comment.
I wrote this script myself (with some Googling to help me with syntax). I can definitely add test cases to this, and definitely should. Thanks!
|
@kelloggm Sorry, I’m a little confused as to how we can achieve this.
Since the two scripts run on different OSes, how can we compare their outputs? Would we run it on Windows and then include a check in the CI? Aside from that I’d definitely be happy to set it up. |
I think what I would do is add a second job in CI that runs on Windows. It would check that 1) the tests pass on Windows as expected, and 2) the results on Windows are the same as the ones on the Linux CI job (not sure how easy that is in GitHub Actions, so we could skip it if we have to) |
Sounds great! I added some test cases in Also, I didn't include any Java examples in the test case because running |
That seems fine to me. |
|
This looks good to me, but there is still one thing to do: add a GitHub actions job that runs the tests on Windows (and the tests you wrote for the Windows diff script, too). We can do that by creating a parallel GitHub actions job. I'd expect that test to 1) run on Windows, and 2) test three things:
|
1 similar comment
|
This looks good to me, but there is still one thing to do: add a GitHub actions job that runs the tests on Windows (and the tests you wrote for the Windows diff script, too). We can do that by creating a parallel GitHub actions job. I'd expect that test to 1) run on Windows, and 2) test three things:
|
|
@kelloggm workflow created and runs successfully. Please let me know if you would like me to change anything else! |
This PR adds 4/20 of the `specimin-evaluation` test cases to the Windows CI to ensure that `main.py` successfully executes on various types of test cases on Windows. - cf-1291: test typical integration test - cf-577: download + unzip + rename/file access errors - cf-691: download + unzip 2 - cf-4614: slightly different than expected CF outputs This also fixes a silent error in `typecheck_test_outputs.bat` from #319 where not all test cases were being checked. The reason behind this was how I was handling `continue` in batch; I used `goto` within a `for` loop, which I did not know broke the script out of the loop context (hence stopping at the test case `methodref2`). The CI should pass once the companion PR njit-jerse/specimin-evaluation#3 is approved; right now we should expect at least a few of these tasks to fail.
This PR creates the
check_differences.batfile to substitute the lack of thediffcommand on Windows, allowing for full Windows support when running./gradlew test. It also portstypecheck_test_outputs.shover to atypecheck_test_outputs.bat, which allows Windows users to call./gradlew expectedTestOutputsMustCompile.