Skip to content
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

Convert default-python integration test to acceptance test #2532

Open
wants to merge 31 commits into
base: main
Choose a base branch
from

Conversation

denik
Copy link
Contributor

@denik denik commented Mar 20, 2025

Changes

  • Convert init_default_python_test.go to acceptance test
  • Record dev targets in addition to prod targets.
  • Record validate json output in addition to summary json.
  • Add venv_activate test helper.

Note, GCP is disabled in this test due to differences from the aws and azure (IsServicePrincipal is false there, causing different config).

Depends on #2535

Why

init_default_python_test.go is one of a kind test - it uses golden files, but it's a separate thing from the rest of acceptance tests. This change makes it easier to update it with the rest of acceptance tests.

@denik denik temporarily deployed to test-trigger-is March 20, 2025 10:40 — with GitHub Actions Inactive
@denik denik force-pushed the denik/integration-acc-default-python branch from 5bea7b2 to 2b9073b Compare March 20, 2025 11:48
@denik denik temporarily deployed to test-trigger-is March 20, 2025 11:48 — with GitHub Actions Inactive
@denik denik changed the base branch from main to denik/acc-fix-diff-py March 20, 2025 11:49
@denik denik force-pushed the denik/integration-acc-default-python branch from 2b9073b to 7deee04 Compare March 20, 2025 12:30
@denik denik temporarily deployed to test-trigger-is March 20, 2025 12:30 — with GitHub Actions Inactive
@denik denik temporarily deployed to test-trigger-is March 20, 2025 12:34 — with GitHub Actions Inactive
@denik denik temporarily deployed to test-trigger-is March 20, 2025 12:38 — with GitHub Actions Inactive
Base automatically changed from denik/acc-fix-diff-py to main March 20, 2025 13:34
@denik denik force-pushed the denik/integration-acc-default-python branch from f95ce63 to 5b97c12 Compare March 20, 2025 13:57
@denik denik temporarily deployed to test-trigger-is March 20, 2025 13:57 — with GitHub Actions Inactive
@denik denik temporarily deployed to test-trigger-is March 20, 2025 14:09 — with GitHub Actions Inactive
@denik denik force-pushed the denik/integration-acc-default-python branch from 4da8eed to 57a5e68 Compare March 20, 2025 15:37
@denik denik temporarily deployed to test-trigger-is March 20, 2025 15:37 — with GitHub Actions Inactive
@denik denik temporarily deployed to test-trigger-is March 21, 2025 10:00 — with GitHub Actions Inactive
@denik denik temporarily deployed to test-trigger-is March 21, 2025 10:04 — with GitHub Actions Inactive
@denik denik temporarily deployed to test-trigger-is March 21, 2025 10:23 — with GitHub Actions Inactive
@denik denik temporarily deployed to test-trigger-is March 21, 2025 11:50 — with GitHub Actions Inactive
@denik denik temporarily deployed to test-trigger-is March 21, 2025 12:20 — with GitHub Actions Inactive
@denik denik temporarily deployed to test-trigger-is March 21, 2025 12:25 — with GitHub Actions Inactive
Copy link
Contributor

@shreyas-goenka shreyas-goenka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me, please take a look at the one comment though.


>>> [CLI] bundle summary -t prod -o json

>>> diff.py ../out.summary.dev.json ../out.summary.prod.json
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is unexpected; shouldn't there be a diff between mode development and production? For mode: development the resources have a prefix added to them.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

well spotted, thanks! diff.py is not doing anything if arguments are files (as opposed to directories).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@denik denik force-pushed the denik/integration-acc-default-python branch from 73fc8b8 to d2c8774 Compare March 21, 2025 15:59
@denik denik temporarily deployed to test-trigger-is March 21, 2025 15:59 — with GitHub Actions Inactive
github-merge-queue bot pushed a commit that referenced this pull request Mar 21, 2025
Copy link
Contributor

@pietern pietern left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

assert d1.is_file(), d1
assert d2.is_file(), d2
diff_files(patterns, d1, d2)
return
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Was this merged in a different PR?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it is rebased now, please take another look.

@denik denik force-pushed the denik/integration-acc-default-python branch from d2c8774 to 374aabf Compare March 25, 2025 16:51
@denik denik temporarily deployed to test-trigger-is March 25, 2025 16:51 — with GitHub Actions Inactive
@denik denik requested review from pietern and shreyas-goenka March 25, 2025 16:55
@denik denik temporarily deployed to test-trigger-is March 25, 2025 17:04 — with GitHub Actions Inactive
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants