Enable test-specific validation bypass with 'testRun' flag on CI runners #1293
Conversation
This file contains hidden or 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
* Initial integration Initial integration for the eim starting from the start to handle the empty workspace. Tools activation is still to be done along with code cleanup. Just the basic display is working which will also be enhanced. * Initial first flow with successful installation and loading of env in IDE with EIM * constants moved to single place and added refresh function * minor cleanup and logging for user * IEP-1334: code cleanup (Active Review) (#1125) * cleanup of unused classes and code * String internalization * update to ci file * ci error fixed * ci error fix * ci fix * fixing ci error * ci error fix * ci error fix * ci syntax fix * ci syntax indentation fix * ci syntax fix * ci syntax fix * ci fixed * ci fixed2 * ci trigger * workflow trigger fix * cleanup * ci deps isntalled * eim fix * using eim action to install * using Petr's fork * switched back to official action repo * Revert "switched back to official action repo" This reverts commit f8cd7a7. * trying to fix action for eim * trying with petrs fork again * ci fix * back to espressif * name fixed * updated url for eim * old config export handling * fixing tests env setup * logging to verify skip tests * fixing POSIX path resolution * activation script properly sourced * added test run variable in test pom directly * adding cache option for the maven * file name updated * increasing timeout for tests * test fixes and removing redundant and outdated tests * cleanup and update for user messages * updated to handle activation of single esp-idf from eim * removing unwanted test for fixing failures * increased timeout for tests * ci updated for the release branch trigger as well * ci error fix * fix for sdkconfig server * cleaning up the idf_tools_path configuration in the IDE * added the option to include homebrew paths on macos * changes for guide link * Guidance link added to NLS * fix: Launch ESP-IDF Manager view even there is no eim_idf.json file found * fix: Don't launch multiple ESP-IDF Manager editors * fix: Update the msg and convert it to MessageDialog * fix: notify IDF not found while opening Manager view * fix: java.lang.UnsupportedOperationException * fix: File Not found issue and others * updated code for eim watcher added Next commits will do some refactoring and then implement the logic to handle these changes * refactored startup classes * initial base level logic for file watcher So this is very basic first level of file watcher service that watches the file and then also takes care that the file is wasnt changes since last startup. Initial changes only take that the file last modified is verified and just any change in file simply will give you a message box telling that something changed. More advanced and robust checks in upcoming commits * missing copyright added --------- Co-authored-by: Kondal Kolipaka <kondal.kolipaka@espressif.com>
* automatic refresh added * switched to message dialog * improved null checks * file watcher added first
…df.json (#1218) * eim auto import for old config added
* IEP-1552: Fix for system python executable
IEP-1554: Launch and Download of EIM added --------- Co-authored-by: Kondal Kolipaka <kondal.kolipaka@espressif.com>
* IEP-1557: Documentation for the ESP-IDF Manager * version.js added
* IEP-1585: Fixed eim old config import handling
fix: added global modal lock to avoid multiple modals
* IEP-1583: Closing old editor window when old workspace is found * fix: address NPE while launching --------- Co-authored-by: Kondal Kolipaka <kondal.kolipaka@gmail.com>
* ci: update the beta workflow and merge release&beta ci
This reverts commit 8b6303a.
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughReplaces legacy in-IDE tools management with EIM-based installation/management, adds EIM loader, parser, watcher, and startup flows; removes prior installers/wizards; updates environment handling to System.getenv; adjusts UI/editor to EIM; modernizes CI (triggers, actions, beta handling); bumps versions to 4.0.0; updates docs/tests accordingly. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant IDE as IDE (Eclipse)
participant Startup as EspressifToolStartup
participant EIM as EIM (Manager)
participant FS as eim_idf.json
participant Core as SetupToolsInIde
User->>IDE: Launch IDE
IDE->>Startup: earlyStartup()
Startup->>Startup: Check EIM installed / JSON present
alt EIM not installed
Startup->>EIM: Download & Launch (EimLoader)
EIM-->>Startup: Exits after install
end
Startup->>FS: Read EIM JSON (EimIdfConfiguratinParser)
alt Single IDF detected
Startup->>Core: setupTools(idfInstalled, eimJson)
Core-->>Startup: OK / Error (rollback on error)
else Multiple/none
Startup->>IDE: Open ESP‑IDF Manager Editor
end
note over IDE,FS: EimJsonWatchService monitors eim_idf.json and prompts to refresh on changes
sequenceDiagram
participant Watcher as EimJsonWatchService
participant FS as eim_idf.json
participant UI as EimJsonUiChangeHandler
participant Editor as ESPIDFManagerEditor
participant Core as SetupToolsInIde
FS-->>Watcher: Modify (create/delete/change)
Watcher->>UI: onJsonFileChanged(path, paused=false)
UI->>UI: Ask user to refresh (modal)
alt Confirm
UI->>FS: Reload EIM JSON
alt Single IDF
UI->>Core: setupTools(...)
else Multiple
UI->>Editor: Open manager and refresh
end
else Decline
UI-->>Watcher: No action
end
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120+ minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
✨ Finishing Touches🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
alirana01
requested changes
Aug 19, 2025
Collaborator
alirana01
left a comment
alirana01
left a comment
There was a problem hiding this comment.
There are some changes that are required kindly go over it and another important thing before we do any of these changes is to pin point all the areas that are required to be modified for test runs. We can then come up with implementation plan for these kind of classes or how to avoid certain parts of these things maybe we will have to split the logic into more classes for Startup or move some to Utility classes and avoid the test run there.
Comment on lines
312
to
319
| Display.getDefault().syncExec(() -> { | ||
| String testRunValue = System.getProperty("testRun"); | ||
| Logger.log("testRun: " + testRunValue); | ||
|
|
||
| if (!StringUtil.isEmpty(testRunValue) && Boolean.parseBoolean(testRunValue)) | ||
| { | ||
| return; | ||
| } |
Collaborator
There was a problem hiding this comment.
Do we need to run this code in Display thread? I think we can try to get the value without it or do you get any errors?
Collaborator
Author
There was a problem hiding this comment.
separated non-UI operation from syncExec()
|
|
||
| if (!StringUtil.isEmpty(testRunValue) && Boolean.parseBoolean(testRunValue)) | ||
| { | ||
| return; |
Collaborator
There was a problem hiding this comment.
Also I am not sure how this is tested or working because this is just returning from the lambda block not the function!
.github/workflows/ci.yml
Outdated
|
|
||
| - name: Build with Maven | ||
| run: export NO_AT_BRIDGE=1 && mvn clean verify -Djarsigner.skip=true -DskipTests=false -DtestWorkspace=/opt/actions-runner/_work/workspace | ||
| run: export NO_AT_BRIDGE=1 && mvn clean verify -Djarsigner.skip=true -DskipTests=false -DtestRun=TEST -DtestWorkspace=/opt/actions-runner/_work/workspace |
Collaborator
There was a problem hiding this comment.
we dont need an additional variable since we already have skipTests we can use that instead of adding a new one
.github/workflows/ci_windows.yml
Outdated
|
|
||
| - name: Build with Maven | ||
| run: mvn clean verify "-Djarsigner.skip=true" "-DskipTests=false" "-DtestWorkspace=C:/actions-runner/_work/workspace" | ||
| run: mvn clean verify "-Djarsigner.skip=true" "-DskipTests=false" -DtestRun=TEST "-DtestWorkspace=C:/actions-runner/_work/workspace" |
Collaborator
There was a problem hiding this comment.
similarly here we dont need an additional variable since we already have skipTests we can use that instead of adding a new one
…c, separate non-UI operations from syncExec
alirana01
force-pushed
the
release/v4.0.0
branch
from
fa28fd9 to
2d49c6e
Compare
alirana01
force-pushed
the
release/v4.0.0
branch
from
3875d67 to
f565735
Compare
Collaborator
|
Hi @AndriiFilippov Looks like this PR has all the 4.0.0 changes together with 37 commits, this only leads to conflicting changes and merge issues, can we make the specific PR for the changes you're working on. |
kolipakakondal
requested changes
Nov 19, 2025
Collaborator
kolipakakondal
left a comment
kolipakakondal
left a comment
There was a problem hiding this comment.
Please re-work on the clear PR as mentioned.
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.
Description
Implementing a conditional check to read a system property
testRunand use it to optionally skip certain validations.Type of change
Please delete options that are not relevant.
How has this been tested?
I have tested implemented code locally on my Windows 11 machine by running:
mvn clean install -Djarsigner.skip=true -DskipTests=false -DtestRun=TESTValidations controlled by the 'testRun' flag were skipped during the run ✅
All tests passed successfully ✅
Test Configuration:
Checklist
Summary by CodeRabbit
New Features
Refactor
CI
Documentation
Tests