fix: server and test stability #313
Conversation
stevenh
force-pushed
the
chore/build-server2
branch
from
a268af9
to
fffcd5f
Compare
stevenh
changed the title
stevenh
force-pushed
the
chore/build-server2
branch
from
fffcd5f
to
1970eae
Compare
|
Sods law, running clean here and total failure in CI, think its the server tests, will look more. |
stevenh
force-pushed
the
chore/build-server2
branch
from
1970eae
to
033404f
Compare
|
Nope looks like I used a feature of Java 11+ so all was exploding |
Significant refactor of the server to improve stability particularly to address hang on shutdown which was causing significant test instability. Switch the logger for logback which adds the ability to do file and console logging and more making it easier to debug issues. Increase max JVM version which was also contributing to server test instability. Reduce ping timeout from 20 to 10 seconds now the server is more stable, adding in a retry if we get disconnected. Enable server build cleanup. Move server tests to their own file as they have been the flaky ones, so its easier if they are grouped together. Move beforeEachTestCase into each test which needs it as beforeEach only gets run from the CLI meaning UI tests were broken.
stevenh
force-pushed
the
chore/build-server2
branch
4 times, most recently
from
2c1ccb8
to
3ddda33
Compare
Switch to using a matrix strategy for the test workflow to simplify and make it obvious what versions we're testing as they are all at the top of each test. This increases the minimum Java version we test on to 17 LTS and adds 21 LTS. This means the current runs will be the following: Tests: * os: ubuntu-latest * node: 18, java: 21 temurin * node: 18, java: 21 adopt * node: 18, java: 17 temurin * os: macos-latest, node: 18, java: 21 temurin * os: windows-latest, node: 18, java: 21 temurin Coverage report: * os: debian-bookworm, node: 18, java: none
stevenh
force-pushed
the
chore/build-server2
branch
from
3ddda33
to
fe5fc5d
Compare
To be consistent and its shorter, rename dev:kill-server to server:kill.
As per best practice remove the use of lambdas from tests: https://mochajs.org/#arrow-functions Also add retries to server tests as they can be racy due to the interaction with the server.
Ensure that each test which uses copyFilesInTmpDir always removes the temporary directory to prevent other tests randomly failing due to additional files being linted. Also fix incorrect count in GroovyDisableLineAll test.
stevenh
force-pushed
the
chore/build-server2
branch
from
f9a05c5
to
3793a19
Compare
Wait for the server to stop processing when we kill it to avoid the next request from failing to restart the server due to a port in use.
stevenh
force-pushed
the
chore/build-server2
branch
from
3793a19
to
2bbf46d
Compare
|
@stevenh do I wait for this PR to be merged before releasing a new version of npm-groovy-lint ? |
|
There are already some good fixes in, can you retry the following to see if we can get those in: We can then do this one in a separate release |
|
@stevenh please let me know when you feel confident enough so I can release a new version then apply it to VsCode GroovyLint :) If it's just one random test failure, you might consider disabling it on some contexts/platforms if you think it's really random and won't happen often on user configurations |
|
I think we have to be pragmatic, as current version of npm-grovvy-lint fails almost everywhere, i prefer to release a version with a few bugs that we'll solve later, than a version not working at all like the current one ^^ Do I have your go to release from |
|
Unfortunately, it seems to affect all tests randomly. I believe it's to do with the server starting and stopping while in the middle of the test run; which is why I've been working on ensure the server is doing as intended when kill is requested. I've managed to get a reproducible scenario locally, but just haven't time to dig in this week. |
|
The current, main should be better than we had and it has the IPv4 fix in, which is one of the more important ones, so if you can test with a vscode build along side the current main and it works, then go for it. |
stevenh
force-pushed
the
chore/build-server2
branch
2 times, most recently
from
0522c68
to
fe32f27
Compare
|
Making some good progress here, this is now a combo of the other PR's which have some dependencies when it comes to the tests. I've had several clean runs on Java 17 both locally and here, but seeing errors on Java 21. Will continue to investigate, so if you do want to hold off the release until next week, I may be able to crack it. |
|
I think I may have a break through, recommend holding fire. |
|
Ok I hold fire :) ( and order a golden statue with your face for saving npm-groovy-lint :D ) |
Skip some unstable locals tests, to see if this is the only CI test failure cases.
Use the Remaining values instead of Found values so that exit status is correct when processing fixes. Also output the full summary structure which provides useful information when we trigger the status 1.
Reset the exit status code between each request so that we correctly report the state of the request not an old one.
Java 21 can't run groovy due to: Unsupported class file major version 64. The supported Java version is now just 17 LTS as while 8 LTS compiles it fails to run with a class not found exception when# running: private static final Logger LOGGER = LoggerFactory.getLogger(CodeNarcServer.name)
stevenh
force-pushed
the
chore/build-server2
branch
from
9b159d7
to
3f0b74a
Compare
|
Ok it's not how I would want it, all in one PR, but this solves all but one of the issues I've seen. Seems Java 8 fails on reflection and Java 21 is not supported due to lack of ASM support. I've marked the unstable tests as skip, but this should put us in a good place for a release. |
Remove old simple logger.
Significant refactor of the server to improve stability particularly to address hang on shutdown which was causing significant test instability.
Switch the logger for logback which adds the ability to do file and console logging and more making it easier to debug issues.
Increase max JVM version which was also contributing to server test instability.
Reduce ping timeout from 20 to 10 seconds now the server is more stable, adding in a retry if we get disconnected.
Enable server build clean up.
Rename dev:kill-server to server:kill to be consistent with other jobs.
To improve test stability: