-
Notifications
You must be signed in to change notification settings - Fork 66
ci: tidy3d-extras integration tests and docs/dev setup (FXC-4441) #3046
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
1 file reviewed, no comments
Diff CoverageDiff: origin/develop...HEAD, staged and unstaged changesNo lines with coverage information in this diff. |
9921fa1 to
a9e8197
Compare
37fca68 to
351ef04
Compare
351ef04 to
75a0f53
Compare
caseyflex
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice! The docker documentation seems helpful and good to have integration tests. A few comments
.github/workflows/tidy3d-extras-python-client-integration-tests.yml
Outdated
Show resolved
Hide resolved
|
@greptile re review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
4 files reviewed, no comments
2645fdd to
74a2b41
Compare
|
@caseyflex I've implemented your suggestions and moved the license_test_ci.py to this repo. I'm getting some weird multiplatform errors ( ubuntu and windows are fine, mac are not, maybe it's some issues on tidy3d extras we haven't caught yet? https://github.com/flexcompute/tidy3d/actions/runs/19959108730/job/57234958887 I'll have time to work on this later today but need to do some other stuff for now, just thought to let you know in case it's clearer to you what's going on |
74a2b41 to
ddeb514
Compare
|
@daquinteroflex is it still broken on those platforms? |
|
Yeah haven't been able to get back to it, find it weird it works for ubuntu but not mac if its the same action config? Looks like an issue with the extension? |
Yes this is a good point. So how about we do full integration tests once a day for all platforms as part of the daily python client tests? But then we have this minimal test suite as it is now for the PR integration tests. Just feeling if we did it per PR then it's probably too much CI/CD overhead to check everything.
So this is an interesting testing question. In the backend we want to check that any changes to tidy3d-extras does not break with tidy3d_frontend. So in your view the tidy3d-extras standalone tests won't catch this? Isn't there an equivalent amount of risk of the python version breaking tidy3d/tidy3d-extras integration on both repos? It sounds like maybe we need to split things into comprehensive daily tests on both repos, but then have a minimal set of smoke tests for integration |
fd623d4 to
37a49de
Compare
ok!
There is a backend test than runs the frontend tests with extras installed. I had some issues with the frontend tests previously that this caught. But these weren’t related to python version. But we could still have that test run multiple python versions, or include that in the full integration tests here Yeah agreed overall |
5923426 to
b793dc4
Compare
|
@greptile review again @caseyflex I've addressed your suggestions fyi, we now have full and basic integration tests. Full tests run part of the autorelease / daily tests |
|
Testing again: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
9 files reviewed, 2 comments
.github/workflows/tidy3d-extras-python-client-tests-integration.yml
Outdated
Show resolved
Hide resolved
.github/workflows/tidy3d-extras-python-client-tests-integration.yml
Outdated
Show resolved
Hide resolved
|
@caseyflex looks like having full native container compatibility is an endeavour of its own, I'm going to change the full tests to ones we know would pass and are more comprehensive for now. https://github.com/flexcompute/tidy3d/actions/runs/20078471065/job/57599207343 We can merge this for now and work out how to best test in the different systems in a separate PR |
dbd6e6d to
08ca7f9
Compare
|
Ok testing again removing the docker containers for a different PR: @greptile review again |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
9 files reviewed, no comments
|
@caseyflex looks like there still may be some issues https://github.com/flexcompute/tidy3d/actions/runs/20107922131/job/57697019013 despite the fix introduced in the build |
4d8266e to
6a2c04b
Compare
6a2c04b to
dd0eae3
Compare
|
@caseyflex alas it was a short lived hope. https://github.com/flexcompute/tidy3d/actions/runs/20131552128/job/57774016801 now it's building well, and license check is passing but it's crashing I think in a subpixel operation |
Greptile Overview
Greptile Summary
This PR introduces comprehensive
tidy3d-extrasintegration testing and Docker-based development environment setup. The changes add a new CI workflow for testing the optional extras package across multiple platforms and Python versions, with two test modes: basic (4 configurations for fast smoke testing on PRs) and full (10 configurations covering all architectures for releases). The PR also includes a complete Docker development environment with detailed documentation.Major Changes:
tidy3d-extras-python-client-tests-integration.ymlworkflow with dual test modes (basic/full) covering Windows, Linux x86_64/aarch64, and macOS x86_64/arm64 on Python 3.10 and 3.13daily-0.0.0) instead of just submodule tests, providing broader CI validationdev.Dockerfileproviding isolated development environment with Poetry, AWS CLI, and development toolsdocs/development/docker.rst) with setup instructions, troubleshooting, and AWS CodeArtifact authentication_test_tidy3d_extras_license.pyto verify license validation and error handling_test_tidy3d_extras_license.pyto prevent redundant execution (this test is run separately in the extras integration workflow)Architecture:
The extras integration tests are invoked through the main test workflow, which determines the test type based on the event trigger. PRs and merge queue events use "basic" mode for fast feedback, while the release workflow uses "full" mode for comprehensive validation before deployment.
Confidence Score: 5/5
Important Files Changed
File Analysis
tidy3d-extrasintegration testing with basic (4 configs) and full (10 configs) test modes. Well-structured with proper authentication and environment setup.extras_integration_testssupport andtest_typeparameter. Tests now properly exclude_test_tidy3d_extras_license.pyto avoid redundant execution.tidy3d-extraslicense validation and error handling with bad API keys. Well-structured with platform compatibility checks.Sequence Diagram
sequenceDiagram participant PR as Pull Request participant MQ as Merge Queue participant Tests as tidy3d-python-client-tests participant Extras as tidy3d-extras-integration-tests participant Release as tidy3d-python-client-release participant Daily as tidy3d-python-client-daily Note over PR,Daily: PR Flow (Basic Tests) PR->>Tests: Trigger on PR Tests->>Tests: Determine test scope<br/>(extras_integration_tests=true, test_type=basic) Tests->>Extras: Call with test_type=basic Extras->>Extras: Run 4 test configurations<br/>(Linux x64, macOS arm64, Windows x64 3.10/3.13) Extras-->>Tests: Return success status Tests-->>PR: Report results Note over PR,Daily: Merge Queue Flow (Basic Tests) MQ->>Tests: Trigger on merge_group Tests->>Tests: Determine test scope<br/>(extras_integration_tests=true, test_type=basic) Tests->>Extras: Call with test_type=basic Extras->>Extras: Run 4 test configurations Extras-->>Tests: Return success status Tests-->>MQ: Report results Note over PR,Daily: Release Flow (Full Tests) Release->>Release: Determine workflow scope<br/>(run_extras_integration_tests=true) Release->>Tests: Call with extras_integration_tests=true,<br/>test_type=full Tests->>Extras: Call with test_type=full Extras->>Extras: Run 10 test configurations<br/>(All platforms x Python 3.10/3.13) Extras-->>Tests: Return success status Tests-->>Release: Report results Note over PR,Daily: Daily Flow (Full Tests via Draft Release) Daily->>Release: Call with release_tag=daily-0.0.0,<br/>release_type=draft Release->>Tests: Call with extras_integration_tests=true,<br/>test_type=full Tests->>Extras: Call with test_type=full Extras->>Extras: Run 10 test configurations Extras-->>Tests: Return success status Tests-->>Release: Report results Release-->>Daily: Complete without publishing