🤖 Fix CI E2E harness bugs and shell compatibility issues#555
Conversation
This commit fixes several issues in the E2E test harness that were causing CI failures and preventing some tests from running: 1. Fixes the EXIT trap in generateDataAndRun.sh to append '|| true' to the kill command, preventing exit code pollution when the monitor is already dead. 2. Replaces 'source' with '.' in all NVM loading commands in python generators and genData100.sh to ensure compatibility with 'dash' (the default /bin/sh in Ubuntu CI). 3. Fixes a mismatched format string bug in message_fmt2.py and datetime_fmt.py logging calls to prevent TypeErrors during error reporting. 4. Removes dead code in relativedatetime_fmt.py and fixes a redundant assignment typo in list_fmt.py. Co-authored-by: Gemini <176961590+gemini-code-assist[bot]@users.noreply.github.com>
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request replaces the source command with . for sourcing nvm.sh across multiple scripts and generator files, fixes several logging format strings, removes redundant code, and cleans up a duplicate assignment. The review feedback recommends using the NVM_DIR environment variable with a fallback instead of hardcoding ~/.nvm/nvm.sh to support custom NVM installation paths (particularly in CI environments). Additionally, it is suggested to use logging.exception instead of logging.error in datetime_fmt.py to automatically capture tracebacks during file-saving failures.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| generate_command = '. ~/.nvm/nvm.sh; nvm install %s; nvm use %s --silent; npm ci; node generators/datetime_gen.js %s %s' % ( | ||
| nvm_version, nvm_version, '-run_limit', self.run_limit) |
There was a problem hiding this comment.
Instead of hardcoding ~/.nvm/nvm.sh, use the NVM_DIR environment variable (with a fallback to ~/.nvm). In CI, NVM_DIR is configured to $HOME/.config/nvm (see generateDataAndRun.sh line 56), so hardcoding ~/.nvm/nvm.sh will fail to locate NVM.
| generate_command = '. ~/.nvm/nvm.sh; nvm install %s; nvm use %s --silent; npm ci; node generators/datetime_gen.js %s %s' % ( | |
| nvm_version, nvm_version, '-run_limit', self.run_limit) | |
| generate_command = '. "${NVM_DIR:-$HOME/.nvm}/nvm.sh"; nvm install %s; nvm use %s --silent; npm ci; node generators/datetime_gen.js %s %s' % ( | |
| nvm_version, nvm_version, '-run_limit', self.run_limit) |
| generate_command = '. ~/.nvm/nvm.sh; nvm install %s; nvm use %s --silent; %s' %\ | ||
| (nvm_version, nvm_version, ' '.join(exec_list)) |
There was a problem hiding this comment.
Instead of hardcoding ~/.nvm/nvm.sh, use the NVM_DIR environment variable (with a fallback to ~/.nvm) to support custom NVM installation paths (such as $HOME/.config/nvm used in CI).
| generate_command = '. ~/.nvm/nvm.sh; nvm install %s; nvm use %s --silent; %s' %\ | |
| (nvm_version, nvm_version, ' '.join(exec_list)) | |
| generate_command = '. "${NVM_DIR:-$HOME/.nvm}/nvm.sh"; nvm install %s; nvm use %s --silent; %s' %\ | |
| (nvm_version, nvm_version, ' '.join(exec_list)) |
| generate_command = '. ~/.nvm/nvm.sh; nvm install %s; nvm use %s --silent; %s' %\ | ||
| (nvm_version, nvm_version, ' '.join(exec_list)) |
There was a problem hiding this comment.
Instead of hardcoding ~/.nvm/nvm.sh, use the NVM_DIR environment variable (with a fallback to ~/.nvm) to support custom NVM installation paths (such as $HOME/.config/nvm used in CI).
| generate_command = '. ~/.nvm/nvm.sh; nvm install %s; nvm use %s --silent; %s' %\ | |
| (nvm_version, nvm_version, ' '.join(exec_list)) | |
| generate_command = '. "${NVM_DIR:-$HOME/.nvm}/nvm.sh"; nvm install %s; nvm use %s --silent; %s' %\ | |
| (nvm_version, nvm_version, ' '.join(exec_list)) |
| generate_command = '. ~/.nvm/nvm.sh; nvm install %s; nvm use %s --silent; %s' %\ | ||
| (nvm_version, nvm_version, ' '.join(exec_list)) |
There was a problem hiding this comment.
Instead of hardcoding ~/.nvm/nvm.sh, use the NVM_DIR environment variable (with a fallback to ~/.nvm) to support custom NVM installation paths (such as $HOME/.config/nvm used in CI).
| generate_command = '. ~/.nvm/nvm.sh; nvm install %s; nvm use %s --silent; %s' %\ | |
| (nvm_version, nvm_version, ' '.join(exec_list)) | |
| generate_command = '. "${NVM_DIR:-$HOME/.nvm}/nvm.sh"; nvm install %s; nvm use %s --silent; %s' %\ | |
| (nvm_version, nvm_version, ' '.join(exec_list)) |
| self.saveJsonFile(dt_verify_path, verify_obj, indent=2) | ||
| except BaseException as err: | ||
| logging.error('!!! %s: Failure to save file %s', err, ) | ||
| logging.error('!!! %s: Failure to save files: %s and %s', err, dt_test_path, dt_verify_path) |
There was a problem hiding this comment.
Using logging.exception is preferred over logging.error when logging from within an except block. It automatically captures and logs the traceback, which is extremely helpful for debugging failures in CI.
| logging.error('!!! %s: Failure to save files: %s and %s', err, dt_test_path, dt_verify_path) | |
| logging.exception('Failure to save files: %s and %s', dt_test_path, dt_verify_path) |
There was a problem hiding this comment.
Pull request overview
This PR aims to stabilize the CI E2E test harness by fixing shell/NVM loading issues, preventing CI-exit-status pollution, and correcting logging-format bugs that can crash generators during error reporting.
Changes:
- Adjusts NVM initialization commands to improve shell compatibility in multiple Node-based data generators and helper scripts.
- Fixes EXIT trap behavior in the harness script to avoid altering the script’s final exit code.
- Corrects Python logging format-string mismatches and removes/cleans up minor generator code issues.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
testgen/generators/segmenter.py |
Updates NVM load command used to run the Node-based segmenter generator. |
testgen/generators/relativedatetime_fmt.py |
Removes dead code and updates NVM load command for the Node-based relative datetime generator. |
testgen/generators/message_fmt2.py |
Fixes logging format string mismatch during schema validation error reporting. |
testgen/generators/list_fmt.py |
Updates NVM load command and fixes a redundant assignment typo. |
testgen/generators/datetime_fmt.py |
Fixes a logging format bug and updates NVM load command for Node-based generation. |
generateDataAndRun.sh |
Prevents EXIT-trap kill failure from polluting the script’s exit status. |
genData100.sh |
Replaces source with . for NVM loading in the helper script. |
Comments suppressed due to low confidence (1)
testgen/generators/segmenter.py:42
subprocess.run(..., shell=True)uses/bin/shby default (typicallydashon Ubuntu CI), butnvmexpects a bash-compatible shell. Also, hard-coding~/.nvm/nvm.shcan break CI becausegenerateDataAndRun.shcan setNVM_DIRto a different location (e.g.$HOME/.config/nvm). Run under bash explicitly, respect$NVM_DIR, and usecheck=Trueso failures don’t get silently ignored.
generate_command = '. ~/.nvm/nvm.sh; nvm install %s; nvm use %s --silent; %s' %\
(nvm_version, nvm_version, ' '.join(exec_list))
logging.debug('Running this command: %s', generate_command)
result = subprocess.run(generate_command, shell=True)
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| generate_command = '. ~/.nvm/nvm.sh; nvm install %s; nvm use %s --silent; %s' %\ | ||
| (nvm_version, nvm_version, ' '.join(exec_list)) | ||
|
|
||
| logging.debug('Running this command: %s', generate_command) | ||
| result = result = subprocess.run(generate_command, shell=True) | ||
| result = subprocess.run(generate_command, shell=True) |
| nvm_version = icu_nvm_versions[self.icu_version] | ||
| generate_command = 'source ~/.nvm/nvm.sh; nvm install %s; nvm use %s --silent; npm ci; node generators/datetime_gen.js %s %s' % ( | ||
| generate_command = '. ~/.nvm/nvm.sh; nvm install %s; nvm use %s --silent; npm ci; node generators/datetime_gen.js %s %s' % ( | ||
| nvm_version, nvm_version, '-run_limit', self.run_limit) | ||
|
|
||
| result = subprocess.run(generate_command, shell=True) |
| nvm_version = icu_nvm_versions[self.icu_version] | ||
| generate_command = 'source ~/.nvm/nvm.sh; nvm install %s; nvm use %s --silent; %s' %\ | ||
| generate_command = '. ~/.nvm/nvm.sh; nvm install %s; nvm use %s --silent; %s' %\ | ||
| (nvm_version, nvm_version, ' '.join(exec_list)) | ||
|
|
||
| result = subprocess.run(generate_command, shell=True) |
|
The other code review comments are good but they are all pre-existing. |
This PR fixes several issues in the E2E test harness that were causing CI failures and preventing some tests from running:
generateDataAndRun.shto append|| trueto thekillcommand, preventing exit code pollution when the monitor is already dead.sourcewith.in all NVM loading commands in python generators andgenData100.shto ensure compatibility withdash(the default/bin/shin Ubuntu CI). This finally enableslist_fmt,rdt_fmt,segmenter, anddatetime_fmttests to run in CI (they were silently failing to generate data before).message_fmt2.pyanddatetime_fmt.pylogging calls to preventTypeErrorsduring error reporting.relativedatetime_fmt.pyand fixes a redundant assignment typo inlist_fmt.py.These fixes stabilize the CI harness and ensure all tests are actually executed.