chore: improve smokeTests environment validation and logging #1046
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
- Add strict mode with error handling (set -euo pipefail) - Add process cleanup with trap command - Add timeout configuration for application startup - Add detailed logging of test output and termination status
jzvikart
reviewed
Dec 13, 2024
| done | ||
| PROJECT_DIR="$(dirname "$PROJECT_DIR")/.." | ||
| PROJECT_DIR="$(cd "$PROJECT_DIR"; pwd)" | ||
| PROJECT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")/.." && pwd)" |
There was a problem hiding this comment.
The above code can deal with symbolic links, I've been using this template for years in my scripts.
jzvikart
reviewed
Dec 13, 2024
| ( | ||
| # Wait for the ready message | ||
| # Wait for the ready message with timeout | ||
| while true; do |
There was a problem hiding this comment.
IMO we shouldn't be using sleep here. We should fix whatever underlying problem is causing the test to fail without it.
There was a problem hiding this comment.
Agreed - arbitrary sleeps can make tests both slower than necessary and introduce flakiness in my experience. I can create an issue/brainstorm a better termination signal instead
aramxc
changed the title
monilpat
requested changes
Dec 13, 2024
|
Gonna revert until we get a better understanding of test fixes. We have two test failures so far and @odilitime mentioned he may have submitted a PR that instituted a failing test and is talking a look. We don't currently have a rule that all tests must pass in the CI pipeline to be merged in yet although that will come soon |
|
to be clear, I'm not looking into it, I already tried to fix it. I'm just not trusting some of the CI tests until someone fixes them. This PR is fine. |
|
gotcha, my b, cancelling my reversion @odilitime #1051 (comment) apologies @monilpat @aramxc |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Relates to:
N/A
Risks
Low - Changes are isolated to the smoke test script and don't affect production code.
Background
What does this PR do?
Enhances the smoke test script with improved logging, error handling, and process management:
What kind of change is this?
Improvements (misc. changes to existing features)
Documentation changes needed?
My changes do not require a change to the project documentation.
Testing
Where should a reviewer start?
scripts/smokeTests.shDetailed testing steps
pnpm run smokeTestsScreenshots
N/A - Command line tool changes only
Deploy Notes
No special deployment steps required - changes only affect development/testing workflow.
@sol.kinetics