Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update combats.txt #1157

Closed
wants to merge 1 commit into from
Closed

Update combats.txt #1157

wants to merge 1 commit into from

Conversation

jaadams5
Copy link
Contributor

@jaadams5 jaadams5 commented Oct 8, 2022

See https://kolmafia.us/threads/invalid-adventure-area-the-barrel-full-of-barrels.28134/

My local tests now fail enumeratedTypesAreCaseInsensitive() apparently because the barrels are no longer valid as an enumerated type.

I'm not sure this is the right fix and it certainly is not a data file integrity test for combats.txt but since AFAIK the same test fails locally for me but not in the GitHub workflow it undermines my trust in the workflow.

@jaadams5 jaadams5 marked this pull request as ready for review October 8, 2022 19:13
@jaadams5 jaadams5 requested a review from a team as a code owner October 8, 2022 19:13
@codecov
Copy link

codecov bot commented Oct 8, 2022

Codecov Report

Merging #1157 (f2f0a01) into main (aff1a72) will decrease coverage by 0.00%.
The diff coverage is n/a.

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff              @@
##               main    #1157      +/-   ##
============================================
- Coverage     30.73%   30.73%   -0.01%     
+ Complexity    15074    15073       -1     
============================================
  Files          1038     1038              
  Lines        161506   161506              
  Branches      35088    35088              
============================================
- Hits          49639    49633       -6     
- Misses       103012   103016       +4     
- Partials       8855     8857       +2     
Impacted Files Coverage Δ
...sourceforge/kolmafia/textui/langserver/Script.java 81.91% <0.00%> (-3.20%) ⬇️
...eforge/kolmafia/persistence/AdventureDatabase.java 57.76% <0.00%> (-0.73%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update aff1a72...f2f0a01. Read the comment docs.

@jaadams5
Copy link
Contributor Author

jaadams5 commented Oct 8, 2022

Checking case insensitivity for $location ==> expected: <Returned: true> but was: <Invalid adventure area: "The Barrel Full of Barrels"
Returned: true>

@heeheehee-kolmafia
Copy link
Contributor

I can't reproduce when running tests normally, although I can with

$ ./gradlew test --tests=CustomScriptTest.enumeratedTypesAreCaseInsensitive 

This points to your specific tendency of running the tests with forkEvery = 1.

The message you report is from a method of AdventureDatabase that's only invoked from a static block. Thus, in the "typical" scenario, this error message is most likely printed out (and ignored) in the first test case that causes AdventureDatabase.class to be loaded.

@jaadams5
Copy link
Contributor Author

jaadams5 commented Oct 9, 2022

I can't reproduce when running tests normally, although I can with

$ ./gradlew test --tests=CustomScriptTest.enumeratedTypesAreCaseInsensitive 

This points to your specific tendency of running the tests with forkEvery = 1.

The message you report is from a method of AdventureDatabase that's only invoked from a static block. Thus, in the "typical" scenario, this error message is most likely printed out (and ignored) in the first test case that causes AdventureDatabase.class to be loaded.

I'm going to politely disagree if only because I am not running with fork=1. I did so when there was test leakage that we were trying to plug, but since it increases the wall clock run time of testing on my system by a factor of 3 or more I no longer do it casually.

@jaadams5
Copy link
Contributor Author

jaadams5 commented Oct 9, 2022

Because I have been known to make mistakes...

Updated to 26827

Ran gradlew jacocotestreport

`Microsoft Windows [Version 10.0.19044.2006]
(c) Microsoft Corporation. All rights reserved.

C:\KoLmafia>gradlew jacocotestreport
Starting a Gradle Daemon, 1 incompatible Daemon could not be reused, use --status for details

Task :compileJava
Note: Some input files use or override a deprecated API.
Note: Recompile with -Xlint:deprecation for details.
Note: Some input files use unchecked or unsafe operations.
Note: Recompile with -Xlint:unchecked for details.

Task :compileTestJava
Note: Some input files use unchecked or unsafe operations.
Note: Recompile with -Xlint:unchecked for details.

Task :test

CustomScriptTest > enumeratedTypesAreCaseInsensitive() FAILED
org.opentest4j.AssertionFailedError at CustomScriptTest.java:105
Java HotSpot(TM) 64-Bit Server VM warning: Sharing is only supported for boot loader classes because bootstrap classpath has been appended

3675 tests completed, 1 failed, 31 skipped

Task :test FAILED

FAILURE: Build failed with an exception.

  • What went wrong:
    Execution failed for task ':test'.

There were failing tests. See the report at: file:///C:/KoLmafia/build/reports/tests/test/index.html

  • Try:

Run with --stacktrace option to get the stack trace.
Run with --info or --debug option to get more log output.
Run with --scan to get full insights.

BUILD FAILED in 3m 52s
6 actionable tasks: 3 executed, 3 up-to-date

C:\KoLmafia>`

@jaadams5
Copy link
Contributor Author

jaadams5 commented Oct 9, 2022

I did shadowjar, clean and then jacocotestreport and this time the test passed locally. Trying to rerun immediately does nothing.

@heeheehee-kolmafia
Copy link
Contributor

My recollection is that there is a secondary effect at play where Gradle's JUnit test runner will run failing tests at the start of an execution, which will exacerbate this particular failure mode. (I can't find documentation of this, offhand.)

IMO the problem is that the test case is fragile, as it depends on the output matching exactly, up to any side effects such as informational log messages. One possible alternative would be to move this into textui/parsetree/DataTypesTest.java and instead check that

assertEquals(
    DataTypes.parseValue(type, firstValue.toUpperCase(), true),
    DataTypes.parseValue(type, firstValue.toLowerCase(), true));

@gausie gausie deleted the barrell branch October 19, 2022 06:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants