Fixed docker testing #2328
Fixed docker testing #2328
Conversation
cmnrd
added
docker
WalkthroughThe recent updates enhance Docker support across multiple components of the project. These changes include method additions, path resolution adjustments, and Docker integration in test configurations and TypeScript file generation. Key modifications ensure Docker compatibility, streamline Docker-related command executions, and introduce new Docker handling in TypeScript configuration. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CppTest
participant DockerComposeGenerator
participant LFGenerator
participant TSFileConfig
participant TestBase
User->>CppTest: Request for Docker support
CppTest-->>CppTest: Executes supportsDockerOption()
User->>DockerComposeGenerator: Create launcher with Unix paths
DockerComposeGenerator->>DockerComposeGenerator: Use FileUtil.toUnixString() and relativize()
User->>LFGenerator: Generate file config for TypeScript
LFGenerator->>TSFileConfig: Pass Docker parameter to constructor
User->>TSFileConfig: Set Docker property
TSFileConfig->>TSFileConfig: Use setDocker() method
TSFileConfig-->>User: Provide command using Docker
User->>TestBase: Execute Docker-related tests
TestBase-->>TestBase: Simplify Docker command determination, remove DOCKER_RUN_SCRIPT
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (6)
- core/src/integrationTest/java/org/lflang/tests/runtime/CppTest.java (1 hunks)
- core/src/main/java/org/lflang/generator/docker/DockerComposeGenerator.java (1 hunks)
- core/src/main/kotlin/org/lflang/generator/cpp/CppPlatformGenerator.kt (2 hunks)
- core/src/main/kotlin/org/lflang/generator/cpp/CppRos2Generator.kt (4 hunks)
- core/src/main/kotlin/org/lflang/generator/cpp/CppStandaloneGenerator.kt (3 hunks)
- core/src/testFixtures/java/org/lflang/tests/TestBase.java (3 hunks)
Additional comments not posted (10)
core/src/main/kotlin/org/lflang/generator/cpp/CppPlatformGenerator.kt (2)
14-14: Added import fororg.lflang.toUnixString. Ensure it is used appropriately in the file.
28-28: Initialization ofrelativeBinDirwith Unix-style path ensures consistency across environments. Verify that all uses ofrelativeBinDirare consistent with this expectation.Verification successful
The initialization of
relativeBinDirwith a Unix-style path is consistent with its usage across the codebase. All references torelativeBinDirinCppRos2Generator.ktandCppStandaloneGenerator.ktalign with the expectation of Unix-style paths, ensuring consistency across different environments.
core/src/main/kotlin/org/lflang/generator/cpp/CppRos2Generator.ktcore/src/main/kotlin/org/lflang/generator/cpp/CppStandaloneGenerator.ktScripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all usages of `relativeBinDir` to ensure consistency. # Test: Search for `relativeBinDir` usage. Expect: Consistent usage across the file. rg --type kotlin $'relativeBinDir' core/src/main/kotlin/org/lflang/generator/cpp/CppPlatformGenerator.ktLength of output: 203
Script:
#!/bin/bash # Description: Verify all usages of `relativeBinDir` to ensure consistency. # Test: Search for `relativeBinDir` usage across the entire codebase. rg --type kotlin 'relativeBinDir'Length of output: 1127
core/src/integrationTest/java/org/lflang/tests/runtime/CppTest.java (1)
52-55: Addition ofsupportsDockerOptionmethod correctly indicates Docker support for C++ tests. This is a crucial update given the PR's focus on Docker testing.core/src/main/kotlin/org/lflang/generator/cpp/CppRos2Generator.kt (3)
61-61: Modification of thecolconcommand generation to dynamically include arguments is a flexible approach, accommodating various build configurations.
94-100: The updated Docker entry point and executable copying logic are well-defined, ensuring that the Docker container is correctly set up with the necessary executables.
42-42: The conditional path resolution based on Docker settings is a robust approach to handle different environments. Ensure the paths are correctly computed in all scenarios.Verification successful
To ensure the paths are correctly computed in both Docker and non-Docker environments, we need to investigate the context around the two occurrences of
fileConfig.srcGenPath.resolveinCppRos2Generator.kt. Specifically, we should look for any conditions or configurations that handle Docker settings.
The context provided confirms that the path resolution in
CppRos2Generator.ktis correctly conditional based on Docker settings. Specifically, the scriptPath is resolved differently depending on whether Docker is enabled or not.
- When Docker is enabled:
fileConfig.srcGenPath.resolve(relativeBinDir).resolve(fileConfig.name)- When Docker is not enabled:
fileConfig.binPath.resolve(fileConfig.name)This ensures that the paths are correctly computed in both Docker and non-Docker environments.
core/src/main/kotlin/org/lflang/generator/cpp/CppRos2Generator.kt:
- Line 42:
fileConfig.srcGenPath.resolve(relativeBinDir).resolve(fileConfig.name)Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the correct computation of paths based on Docker settings. # Test: Check if the paths resolve correctly in both Docker and non-Docker environments. rg --type kotlin $'fileConfig.srcGenPath.resolve' core/src/main/kotlin/org/lflang/generator/cpp/CppRos2Generator.ktLength of output: 284
Script:
#!/bin/bash # Description: Search for Docker-related conditions or configurations in CppRos2Generator.kt. # Test: Look for references to Docker settings in the file. rg --type kotlin 'Docker' core/src/main/kotlin/org/lflang/generator/cpp/CppRos2Generator.ktLength of output: 465
Script:
#!/bin/bash # Description: Search for conditions that might differentiate Docker and non-Docker environments. # Test: Look for if conditions or similar constructs in the file. rg --type kotlin 'if' core/src/main/kotlin/org/lflang/generator/cpp/CppRos2Generator.ktLength of output: 263
Script:
#!/bin/bash # Description: Retrieve the full context of the lines where path resolution occurs. # Test: Print the surrounding lines of the path resolution occurrences. rg --type kotlin -A 5 -B 5 'fileConfig.srcGenPath.resolve' core/src/main/kotlin/org/lflang/generator/cpp/CppRos2Generator.ktLength of output: 1121
core/src/main/java/org/lflang/generator/docker/DockerComposeGenerator.java (1)
132-134: The use ofFileUtil.toUnixStringfor path conversion increateLauncherensures Unix-style path consistency, which is crucial for Docker environments.core/src/main/kotlin/org/lflang/generator/cpp/CppStandaloneGenerator.kt (2)
179-179: Ensure the use ofrelativeBinDiraligns with the refactored path handling in the Docker context.Verification successful
Let's start by verifying the initialization of
relativeBinDirinCppPlatformGenerator.ktand then ensure its consistent usage in the Docker context withinCppStandaloneGenerator.kt.
The initialization of
relativeBinDirinCppPlatformGenerator.ktis consistent and aligns with the expected path handling. Additionally, the usage ofrelativeBinDirwithinCppStandaloneGenerator.ktis consistent and appears correctly integrated with the Docker context.
Initialization of
relativeBinDir:
CppPlatformGenerator.kt:protected val relativeBinDir = fileConfig.outPath.relativize(fileConfig.binPath).toUnixString()Usage in
CppStandaloneGenerator.kt:
"-DCMAKE_INSTALL_BINDIR=$relativeBinDir"defaultEntryPoint():listOf("$relativeBinDir/${fileConfig.name}")generateCopyOfExecutable():COPY --from=builder /lingua-franca/$name/$relativeBinDir/$name ./$relativeBinDir/$nameNo issues found with the use of
relativeBinDirin the Docker context.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that the variable `relativeBinDir` is properly initialized and used across the project. # Test: Search for 'relativeBinDir' usage. Expect: Consistent initialization and usage. rg --type kotlin 'relativeBinDir'Length of output: 1127
Script:
#!/bin/bash # Verify the initialization of `relativeBinDir` in `CppPlatformGenerator.kt` ast-grep --lang kotlin --pattern 'protected val relativeBinDir = $_' --json # Ensure consistent usage in Docker context in `CppStandaloneGenerator.kt` rg --context 5 'relativeBinDir' core/src/main/kotlin/org/lflang/generator/cpp/CppStandaloneGenerator.kt # Confirm Docker-related path handling is correct rg 'Docker' core/src/main/kotlin/org/lflang/generator/cpp/CppStandaloneGenerator.ktLength of output: 2193
Line range hint
195-222: Refactored Docker generator looks clean and well-structured. Ensure that the new Docker commands align with the project's Docker strategy.core/src/testFixtures/java/org/lflang/tests/TestBase.java (1)
573-577: Refactoring of thegetExecCommandmethod to useLFCommandlooks appropriate. Ensure that the command generation and execution are thoroughly tested since they are critical for test reliability.Verification successful
The search results indicate that there are numerous test methods annotated with
@Testin the subclasses ofTestBasewithin thecore/src/integrationTest/java/org/lflang/tests/runtimedirectory. This suggests that thegetExecCommandmethod is indirectly tested through these subclasses.
- core/src/integrationTest/java/org/lflang/tests/runtime/CppTest.java
- core/src/integrationTest/java/org/lflang/tests/runtime/TypeScriptTest.java
- core/src/integrationTest/java/org/lflang/tests/runtime/RustTest.java
- core/src/integrationTest/java/org/lflang/tests/runtime/CppRos2Test.java
- core/src/integrationTest/java/org/lflang/tests/runtime/CZephyrTest.java
- core/src/integrationTest/java/org/lflang/tests/runtime/PythonTest.java
- core/src/integrationTest/java/org/lflang/tests/runtime/CVerifierTest.java
- core/src/integrationTest/java/org/lflang/tests/runtime/CSchedulerTest.java
- core/src/integrationTest/java/org/lflang/tests/runtime/CFlexPRETTest.java
- core/src/integrationTest/java/org/lflang/tests/runtime/CTest.java
- core/src/integrationTest/java/org/lflang/tests/runtime/CCppTest.java
- core/src/integrationTest/java/org/lflang/tests/runtime/CArduinoTest.java
Given this extensive coverage, it is reasonable to conclude that the
getExecCommandmethod is adequately tested through these integration tests.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the correct functionality of `getExecCommand` method in different scenarios. # Test: Search for `getExecCommand` usage in tests. Expect: Proper handling and execution. rg --type java 'getExecCommand'Length of output: 260
Script:
#!/bin/bash # Search for all public methods in TestBase.java and check if they call getExecCommand. ast-grep --lang java --pattern 'public $_ $_($_) { $$$ }' core/src/testFixtures/java/org/lflang/tests/TestBase.javaLength of output: 2055
Script:
#!/bin/bash # Search for instances where TestBase is used in test files. rg --type java 'TestBase'Length of output: 5510
Script:
#!/bin/bash # Search for test methods in the subclasses of TestBase. rg --type java '@Test' core/src/integrationTest/java/org/lflang/tests/runtimeLength of output: 3843
|
Turns out that the TypeScript docker test is failing. I don't understand why, though. It works fine if I compile it with lfc, even if I manually force |
I'm not sure I understand because it seems to me like the tests have been doing something. For example, this comment that you posted earlier @cmnrd shows a runtime error (during dynamic linking) while executing a Docker test with |
There was a problem hiding this comment.
With the changes in this PR, test/C/src/docker/FilesPropertyContainerized does not result in a test failure even when it finishes with a nonzero exit code. You can verify this by adding exit(1) to the startup reaction and running the C Docker tests:
target C {
files: "../include/hello.h",
docker: true
}
preamble {=
#include "hello.h"
=}
main reactor {
reaction(startup) {=
hello_t hello;
exit(1);
printf("SUCCESS\n");
=}
}But in master, this modified version of the test fails with a helpful error message, as it should:
====================================================================
ERROR: One or multiple containers exited with a non-zero exit code.
See the log above for details. The following containers failed:
FilesPropertyContainerized exited with code 1
+---------------------------------------------------------------------------+
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- core/src/main/kotlin/org/lflang/generator/ts/TSFileConfig.kt (2 hunks)
- core/src/main/kotlin/org/lflang/generator/ts/TSGenerator.kt (1 hunks)
Additional comments not posted (4)
core/src/main/kotlin/org/lflang/generator/ts/TSFileConfig.kt (3)
45-45: Added a new optional parameterdockerto the constructor to manage Docker support.This change aligns with the PR's objective to enhance Docker support in the TypeScript generation. Ensure that all instances of
TSFileConfigare updated to handle the new constructor signature where used.
57-59: IntroducedsetDockermethod to dynamically enable or disable Docker support.This method provides flexibility in configuring Docker support at runtime, which is crucial for conditional Docker usage based on environment or other settings.
62-73: RefactoredgetCommandto handle Docker mode dynamically.This refactor enhances the command generation logic to accommodate Docker-based execution paths, which is essential for the Docker integration. The exception handling for undefined Docker mode is a good safety measure.
core/src/main/kotlin/org/lflang/generator/ts/TSGenerator.kt (1)
115-115: Enhanced thedoGeneratemethod to set Docker support based on configuration.This line ensures that Docker settings are properly configured before proceeding with generation, which is critical for maintaining consistent behavior in Docker environments.
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- core/src/main/java/org/lflang/generator/LFGenerator.java (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- core/src/main/java/org/lflang/generator/LFGenerator.java
petervdonovan
force-pushed
the
fix-docker-test
branch
from
759cd9d to
cf6c1f3
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- core/src/main/kotlin/org/lflang/generator/ts/TSFileConfig.kt (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- core/src/main/kotlin/org/lflang/generator/ts/TSFileConfig.kt
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- core/src/main/kotlin/org/lflang/generator/ts/TSFileConfig.kt (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- core/src/main/kotlin/org/lflang/generator/ts/TSFileConfig.kt
lhstrh
dismissed
petervdonovan’s stale review
Pretty sure this has been addressed
I don't fully understand either. It appears that our tests were good at picking up errors where our generated binary fails, but not where the container execution itself fails. The error message that you link to did not show up in testing, but only when I executed the program myself. Similarly, I was struggling with an issue where the entry point in the dockerfile was wrong, but it did not show up in the tests (The output from the execute step was simply empty).
That is an excellent catch! I did not notice that
Nope... |
For some reason, we were not testing the docker generation using the launch scripts that we place in
bin, but instead the test framework would generate a custom docker compose file and run that. This, however, did not work at all. I don't understand the precise reasons, but for the docker tests it would simply do nothing. Thus, a couple of problems in the docker generation were masked, as we never really executed the containers or tested the generated launch script.This PR simply removes the generation of custom docker compose files in the test framework and executes the tests using the generated launch script. This PR also includes a series of fixes for bugs that were uncovered by this change.
Summary by CodeRabbit
New Features
Refactor
Bug Fixes
These changes collectively improve Docker support, streamline file path handling, and refine script generation logic.