search

lf-lang / lingua-franca

Intuitive concurrent programming in any language
https://www.lf-lang.org
Other
234 stars 62 forks source link

Fixed docker testing #2328

Closed cmnrd closed 3 months ago

cmnrd commented 3 months ago

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

These changes collectively improve Docker support, streamline file path handling, and refine script generation logic.

coderabbitai[bot] commented 3 months ago

Walkthrough

The 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

Files Change Summary
core/src/integrationTest/java/org/lflang/tests/runtime/CppTest.java Added supportsDockerOption() method to enable Docker support.
core/src/main/.../lf/lang/generator/docker/DockerComposeGenerator.java Modified createLauncher() for path resolution using Unix string manipulation.
core/src/main/.../generator/cpp/CppPlatformGenerator.kt Introduced toUnixString import and adjusted path conversion with relativeBinDir.
core/src/main/.../generator/cpp/CppRos2Generator.kt Improved file path resolutions, Docker image generation, and colcon command creation.
core/src/main/.../generator/cpp/CppStandaloneGenerator.kt Refined handling of CMake install bindir path, added relativeBinDir, and updated executable generation methods.
core/src/testFixtures/.../org/lflang/tests/TestBase.java Streamlined Docker-related test setup, removed DOCKER_RUN_SCRIPT, simplified execution commands.
core/src/main/.../generator/ts/TSFileConfig.kt Added docker parameter in the constructor, introduced setDocker() method, and updated command execution logic to handle Docker.
core/src/main/.../generator/ts/TSGenerator.kt Set Docker property in fileConfig.
core/src/main/java/org/lflang/generator/LFGenerator.java Added extra argument to TSFileConfig constructor for TypeScript to include Docker parameter.

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?

Share - [X](https://twitter.com/intent/tweet?text=I%20just%20used%20%40coderabbitai%20for%20my%20code%20review%2C%20and%20it%27s%20fantastic%21%20It%27s%20free%20for%20OSS%20and%20offers%20a%20free%20trial%20for%20the%20proprietary%20code.%20Check%20it%20out%3A&url=https%3A//coderabbit.ai) - [Mastodon](https://mastodon.social/share?text=I%20just%20used%20%40coderabbitai%20for%20my%20code%20review%2C%20and%20it%27s%20fantastic%21%20It%27s%20free%20for%20OSS%20and%20offers%20a%20free%20trial%20for%20the%20proprietary%20code.%20Check%20it%20out%3A%20https%3A%2F%2Fcoderabbit.ai) - [Reddit](https://www.reddit.com/submit?title=Great%20tool%20for%20code%20review%20-%20CodeRabbit&text=I%20just%20used%20CodeRabbit%20for%20my%20code%20review%2C%20and%20it%27s%20fantastic%21%20It%27s%20free%20for%20OSS%20and%20offers%20a%20free%20trial%20for%20proprietary%20code.%20Check%20it%20out%3A%20https%3A//coderabbit.ai) - [LinkedIn](https://www.linkedin.com/sharing/share-offsite/?url=https%3A%2F%2Fcoderabbit.ai&mini=true&title=Great%20tool%20for%20code%20review%20-%20CodeRabbit&summary=I%20just%20used%20CodeRabbit%20for%20my%20code%20review%2C%20and%20it%27s%20fantastic%21%20It%27s%20free%20for%20OSS%20and%20offers%20a%20free%20trial%20for%20proprietary%20code)
Tips ### Chat There are 3 ways to chat with [CodeRabbit](https://coderabbit.ai): - Review comments: Directly reply to a review comment made by CodeRabbit. Example: - `I pushed a fix in commit .` - `Generate unit testing code for this file.` - `Open a follow-up GitHub issue for this discussion.` - Files and specific lines of code (under the "Files changed" tab): Tag `@coderabbitai` in a new review comment at the desired location with your query. Examples: - `@coderabbitai generate unit testing code for this file.` - `@coderabbitai modularize this function.` - PR comments: Tag `@coderabbitai` in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples: - `@coderabbitai generate interesting stats about this repository and render them as a table.` - `@coderabbitai show all the console.log statements in this repository.` - `@coderabbitai read src/utils.ts and generate unit testing code.` - `@coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.` - `@coderabbitai help me debug CodeRabbit configuration file.` 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) - `@coderabbitai pause` to pause the reviews on a PR. - `@coderabbitai resume` to resume the paused reviews. - `@coderabbitai review` to trigger an incremental review. This is useful when automatic reviews are disabled for the repository. - `@coderabbitai full review` to do a full review from scratch and review all the files again. - `@coderabbitai summary` to regenerate the summary of the PR. - `@coderabbitai resolve` resolve all the CodeRabbit review comments. - `@coderabbitai configuration` to show the current CodeRabbit configuration for the repository. - `@coderabbitai help` to get help. Additionally, you can add `@coderabbitai ignore` anywhere in the PR description to prevent this PR from being reviewed. ### CodeRabbit Configration File (`.coderabbit.yaml`) - You can programmatically configure CodeRabbit by adding a `.coderabbit.yaml` file to the root of your repository. - Please see the [configuration documentation](https://docs.coderabbit.ai/guides/configure-coderabbit) for more information. - If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: `# yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json` ### Documentation and Community - Visit our [Documentation](https://coderabbit.ai/docs) for detailed information on how to use CodeRabbit. - Join our [Discord Community](https://discord.com/invite/GsXnASn26c) to get help, request features, and share feedback. - Follow us on [X/Twitter](https://twitter.com/coderabbitai) for updates and announcements.
cmnrd commented 3 months ago

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 useHierarchicalBin=true. @petervdonovan is that something you could help with?

petervdonovan commented 3 months ago

This, however, did not work at all. I don't understand the precise reasons, but for the docker tests it would simply do nothing.

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 docker compose, presumably via the Bash script that was in the test framework.

cmnrd commented 3 months ago

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 docker compose, presumably via the Bash script that was in the test framework.

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).

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:

That is an excellent catch! I did not notice that docker compose up would mask those errors. I addressed this in https://github.com/lf-lang/lingua-franca/pull/2330

Pretty sure this has been addressed

Nope...