Prevent testsuite from reading user settings and data #10170
Conversation
Previously, it was ran before each test, but just running it once before all tests is sufficient. So switch from @before to @BeforeClass and make the its result variable static.
This abstracts some the common code (running Arduino, copying output, waiting for completion, checking result) from all testcases into a single method. This simplifies each testcase, but also prepares for adding more common arguments to all runs in a subsequent commit.
In one test, `--get-pref` is passed on the commandline to prevent starting the full GUI. When ran without arguments, `--get-pref` causes *all* preferences to be printed. Using `--version` achieves the same (no GUI is started) with just a single line of output.
This makes the test output a bit more easier to read.
matthijskooijman
force-pushed
the
test-suite-fixes
branch
2 times, most recently
from
b9c5559 to
2e7dc85
Compare
|
Just pushed some more style fixes to satisfy the checks :-) |
Both classes contained some duplicate code, so unify that by making one a subclass of the other. This also prepares for further additions that should be inherited by both.
Previously, this used the DeleteFilesOnShutdown class and a shutdown hook, which would delete the files only after shutdown. However, the shutdown handler would be re-added for every testcase, potentially leading to a lot of threads trying to delete the same files. This uses an alternative: Just keep a list of files to delete inside the testcase and use an @after handler to delete the files directly after each usecase.
Normally, init is only called once during startup, so this does not add anything. However, when running the testsuite, PreferencesData could be initialized multiple times in a single test run. To prevent preferences from a previous test from interfering with subsequent tests, always start with a clean slate when calling init.
The tests would initialize Base, PreferencesData with default settings and/or run the arduino executable, without specifying any settings to use. In practice, this would read settings from e.g. `~/.arduino15`, load libraries from the user's sketchbook, etc. This is not a good idea, since this can be influence the test. For example, the presence of invalid libraries would cause extra output to be generated, which breaks the `--version` test. Or having the "Use external editor" setting set would break tests that try to edit a sketch. This commit fixes this. The core of this commit is in `AbstractWithPreferencesTest`, which sets up a clean settings dir (to replace `~/.arduino15`) with a sketchbook and `preferences.txt` file inside before every test (and removes it again after the test. For some tests, this is enough, but some tests create an instance of `Base`, which again initializes everything, including preferences, from the default location. To prevent that, `--preferences-file` is passed to the `Base` constructor, loading the previously set up preferences. This is handled by the new `AbstractWithPreferencesTest.createBase()` method. Furthermore, CommandLineTest calls the actual Arduino executable, which has the same problem. This is fixed by passing the same `--preferences-file` option on the commandline (generated by `AbstractWithPreferencesTest.getBaseArgs()`). This should prevent all tests from reading the the default settings files, fixing some tests on my system and even speeding up the tests somewhat (due less libraries and cores to load, probably).
This would match the "Close" dialog, by looking for any JDialog on screen. However, in some cases, there could be a second dialog (I have seen a "Install this package to use your xxx board" popup because I happened to have an Arduino Zero connected, presumably an "Updates are available" popup could cause this as well). By matching not just the type of the dialog, but also the title, only one dialog is matched and the testcase runs more reliably.
This prevents the Arduino instance in tests from checking for updates. I have not seen any problems resulting from this, but disabling network requests is generally a good idea in tests (to prevent external factors from influencing the test results). In addition to update checks, there is also the CloudBoardResolver that could do network requests, but there does not seem to be a preference for disabling that.
matthijskooijman
force-pushed
the
test-suite-fixes
branch
from
2e7dc85 to
6473284
Compare
|
I just force pushed this again, I had accidentally pushed some commits in here that were intended for #9954 :-) |
|
✅ Build completed. Please test this code using one of the following: ⬇️ https://downloads.arduino.cc/javaide/pull_requests/arduino-PR-10170-BUILD-963-linux32.tar.xz ℹ️ The |
|
@cmaglie, it seems you've merged this (and other) pull requests by rebasing and without a merge commit. Looking at the resulting git history, that is clean, but also loses information about which commit were grouped into a pull request and the number of that pull request. Maybe it would be better to always use normal merge commits for pull requests instead (unless you squash-and-merge, since than you just get one commit with the PR number added, of course)? |
|
It's not so simple unfortunately, doing always the merge commit (as we did in the past when the squash or rebase merge "button" wasn't available) lead to something like:
that IMHO fails in the goal of grouping commits based on PRs.
The "hard" alternative is to ask the author to rebase the PR just before merging. This will be the best because:
The con is that this is feasible only if the OP is practical with git and he's reactive. We always struggle to ask authors to make clean PRs, so it will be a real pain to also wait the OP to make a rebase before merging. BTW, if the PR is already rebased we can do the merge commit, I agree, I'll keep an eye on this for the next merges. |
|
Well, in a way the grouping is still preserved, but indeed a lot harder to see. If only github had a rebase-and-then-create-merge-commit option... I was going to contact github to suggest it, but a little bit of googling showed that people have already done this: isaacs/github#1143 (also gitlab and bitbucket implement this option, as a nice global option to apply to all merges).
Yeah, that is indeed a problem (and even when it is rebased, then it might be out of date again before you get around to merging it...). Often, you can also do the rebase yourself though (if the author did not uncheck the "allow pushes" checkbox), but that will require manual work (though you often would do a checkout anyway for testing, I guess). Looking further, I found this thread about a request to add a "rebase" button to github (that you can press before merging, and then even wait for CI to complete to check the rebase did not break anything). Among the suggestions there, is this action, which can be used directly to trigger rebases using a |
While trying to add a testcase for #9954, I noticed that some tests were failing on my system. I also noticed that the tests were reading my regular preferences,
~/.arduino15directory and sketchbook. This resulted in test failures, since that caused some "Invalid library" messages to be printed and I had "external editor" set making the IDE editor read-only. But also, this is generally a bad idea, since it makes tests unreliable and might even end up messing with a user's settings and data.Most of this PR are small fixes and refactors, working up to the "Tests: Do not read system user's data" commit that fixes this particular problem. There are also a few commits with small unrelated fixes (which were enabled by these refactorings).
With this PR applied, the testsuite ran correctly, except for the issues caused by #10168.