Skip to content

Conversation

@emizzle
Copy link
Contributor

@emizzle emizzle commented Nov 5, 2025

A number of bugs were encountered in windows CI during development of the parallel integration tests. Fixes for these bugs were ported to this PR without enabling parallel integration tests to see if this alleviates intermittent test failures in windows ci.

Changes include:

  • refactor of integration tests to include manually force killing windows processes, preventing hangs
  • more graceful shutdown, allowing for successful restarts
  • adding unique cache key constraints per workflow and run in an attempt to avoid "illegal instruction" failures in CI.

- need to test with longer tests to ensure the parallelisation is truly happening
- is the +10 hardhat port needed?
- try with more integration tests

# Conflicts:
#	tests/integration/hardhatprocess.nim
#	tests/integration/multinodes.nim
#	tests/integration/testcli.nim
#	tests/testIntegration.nim
# Conflicts:
#	tests/testIntegration.nim
# Conflicts:
#	tests/testIntegration.nim
# Conflicts:
#	tests/testIntegration.nim
# Conflicts:
#	tests/integration/hardhatprocess.nim
prevents showing error in the logs when an expected process exit code is encountered

# Conflicts:
#	tests/integration/testcli.nim
# Conflicts:
#	tests/integration/testcli.nim
# Conflicts:
#	tests/ethertest.nim
- add a TestManager property to IntegrationTest, so manager does not need to be passed into all functions
- cleanup:
  - remove unneeded stopHardhat function
  - add hardhat instance to manager outside of startHardhat
# Conflicts:
#	tests/testIntegration.nim
allows user to know which tests are still running
Work around the AsyncProcess timeout not working correctly
# Conflicts:
#	tests/testIntegration.nim
# Conflicts:
#	tests/integration/hardhatprocess.nim
emizzle and others added 23 commits November 4, 2025 20:18
On windows, termination of hardhat processes would not actually kill the process, and then closing the process' streams would then hang the calling nim process. To get around this, the process is now killed externally using a script, winkillhardhat.sh. This script first queries open processes by inspecting the command line value of all "node.exe" processes, searching for "vendor/codex-contracts-eth" and for the port parameter it was started with. After querying, the process is killed using the `Stop-Process` powershell command (passing the pid of the windows process).
… params of eventually

With this fix in, there is no need to use the asynctest update that sets longer defaults for eventually, so downgrade asynctest
Since the HttpClient now supports async, re-enable debug logging in the Codex nodes
…ebugHardhat=true

Hardhat output is logged to file in hardhat.log for each test, and printing to screen is not necessarily needed as it is already logged to file and can create clutter in the output, so stop writing logging output to memory and writing to screen.
In situations like timeouts, windows will hang when attempting to close the test process streams. In this case, we have to force kill the test process externally. This is the same process as force killing hardhat nodes after they are already terminated, but windows refuses hangs when closing their process streams. This commit creates a forceKillProcess utility that allows a process to be killed by its process name and matching commandline criteria, like TestId (for test process) or --port (for hardhat)
Because `eventuallySafe` calls the symbol `eventually`, it should be declared before `proc eventually` is declared to avoid ambiguous symbol lookups.
Also adds a condition such that a hardhat node is not started for parallel integration tests
It didn't fix the error with invalid proofs in windows in ci
Also, catch exceptions on JsonRpcProvider.close, which should not need to be done, as this routine should not raise any exceptions.
Permission denied creating the symlink in CI
@github-actions github-actions bot added the fix label Nov 5, 2025
@emizzle emizzle changed the title fix(ci): fix window by introducing parallel integration test changes fix(ci): introduce a number of integration test fixes Nov 7, 2025
@emizzle emizzle marked this pull request as ready for review November 7, 2025 04:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants