Test Help

fronobulax

fronobulax

Developer
Staff member
@heeheehee

I looked at the security scan and saw a user controlled file name in Preferences. I thought I would look at it. I made a new local branch. I ran IntelliJ's inspect and found a couple of things I thought I should change and then did so. I then looked at jacoco coverage and found a couple of routines with no coverage. They were private and so I decided to test them implicitly by calling something public that called them. That was the function that actually did IO so I started a new test that set Preferences.saveSettingsToFile=true and used username of my choice. Things were going fine until I looked at test\root\settings and saw a lot of clutter there. I saw the files I expected for my user and GLOBAL_prefs.txt which had the potential to bleed across tests. So I whacked all of the files in test\root\settings. I ran and got test failures from PreferencesTest. I had a WTF moment so I stashed my changes and switched back to main. test\root\settings remained empty, as expected since none of the files are "checked in". gradlew clean and gradle test run so quickly that I don't believe anything was actually running.

I set
org.gradle.caching=false
org.gradle.parallel=false
and tests ran and files were created in test\root\settings

Question 1 - could we have a gradle task the deleted the cache so I don't have to go into gradle.properties to bypass it? Maybe gradle clean invalidates all caches as well?
Question 2 - is it reasonable for me to modify (at least) Preferences tests so that test\root\settings is empty when they are over and perhaps cleared out between tests? That will, at least, convince me that it is not leakage from my new test that broke the old ones.
Question 3 - will I be forgiven if my PR, when finally pushed fails the OS tests? Obviously I would need to fix things before the PR got any serious attention :-)
 
fronobulax said:
Question 1 - could we have a gradle task the deleted the cache so I don't have to go into gradle.properties to bypass it? Maybe gradle clean invalidates all caches as well?
Click to expand...
Probably doable by deleting .cache/ and subdirectories. I don't think there's a different way to do it. (Maybe there's a plugin for it, but doesn't seem worth the effort)

fronobulax said:
Question 2 - is it reasonable for me to modify (at least) Preferences tests so that test\root\settings is empty when they are over and perhaps cleared out between tests? That will, at least, convince me that it is not leakage from my new test that broke the old ones.
Click to expand...
I have no objections to that, or if you wanted to extend the ClearSharedState extension to handle that, or if you wanted to do the same but as part of the gradle test.

fronobulax said:
Question 3 - will I be forgiven if my PR, when finally pushed fails the OS tests? Obviously I would need to fix things before the PR got any serious attention
Click to expand...
I don't care what happens as long as it's not committed :)

But in all seriousness, I think it's fine as long as we're not just disabling tests left and right.
 
In my perfect world gradle would not store things in both my user directory and my KoLmafia directory. Maybe that's a hang over from when I had Gradle installed separately because the wrapper wasn't quite behaving? In any event I know know I have two places to nuke gradle settings.

I'll look at ClearSharedState.

Thanks.
 
What is the usage of ClearSharedState?

I see it as a hook triggered by @AfterAll so I did something like

Code: 
  @AfterAll
  protected static void cleanUp() {
    //exists to trigger hooked routine
  }

in the tests I wanted to clear the state. I'm not seeing the expected cleaning.

Am I thinking this wrong or do I just need to post a PR ?
 
ClearSharedState is registered as an extension, and its afterAll method is run as if it were annotated with @AfterAll for every test suite.
 
PreferencesTest does a few things. Primarily it tests the methods of Preferences.java, but I wrote it because we were having so many failures around preferences prior to ClearSharedState.

PreferencesTest does at least one test with saveSettingsToFile=True, because it's testing saveSettingsToFile. Sorta in the lane for that test.

Do we need a @BeforeAll annotation to check for common state? Or perhaps a per-test-class-home folder?
 
Watch the PR. :-)

I have ideas but how many of them are workable and will I think that way when I have recovered from half-priced growler night?
 
You must log in or register to reply here.
Back
Top