Skip to content

Conversation

@ace-n
Copy link
Contributor

@ace-n ace-n commented Feb 6, 2021

Related to b/173545239 and #1242

@ace-n ace-n requested a review from bshaffer February 6, 2021 00:11
@ace-n ace-n requested a review from a team as a code owner February 6, 2021 00:11
@google-cla google-cla bot added the cla: yes This human has signed the Contributor License Agreement. label Feb 6, 2021
@product-auto-label product-auto-label bot added the samples Issues that are directly related to samples. label Feb 6, 2021
Copy link
Contributor

@bshaffer bshaffer left a comment

Choose a reason for hiding this comment

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

Hmm, so we don't actually want to skip all flex tests. We just want to skip flex DEPLOYMENT tests. I think we could do this by:

  1. Adding a check in each GAE flex deployment test to verify RUN_FLEX_DEPLOYMENT_TESTS should be true. This could be done by adding this to every appengine/flexible/*/test/DeployTest.php file:

    /** @beforeClass */
    public static function checkFlexDeploymentEnvVar()
    {
        self::requireEnv('RUN_FLEX_DEPLOYMENT_TESTS');
    }
  2. Add a block to run_test_suite.sh to temporarily set RUN_DEPLOYMENT_TESTS to false if the directory contains an app.yaml with env: flex. I can't really see a good/elegant way to do that with the current script though, so I prefer option 1

@ace-n
Copy link
Contributor Author

ace-n commented Feb 8, 2021

Add a block to run_test_suite.sh to temporarily set RUN_DEPLOYMENT_TESTS to false if the directory contains an app.yaml with env: flex. I can't really see a good/elegant way to do that with the current script though, so I prefer option 1

We already do this - that's what the grep in run_test_suite.sh is for. 🙂

@ace-n ace-n requested a review from bshaffer February 10, 2021 21:02
Copy link
Contributor

@bshaffer bshaffer left a comment

Choose a reason for hiding this comment

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

Looks great! Should we skip the deployment test in endpoints/getting-started as well? I believe so!

@ace-n ace-n added the automerge Merge the pull request once unit tests and other checks pass. label Feb 11, 2021
@gcf-merge-on-green gcf-merge-on-green bot merged commit 5fdf2bb into master Feb 11, 2021
@gcf-merge-on-green gcf-merge-on-green bot removed the automerge Merge the pull request once unit tests and other checks pass. label Feb 11, 2021
ace-n pushed a commit that referenced this pull request Feb 16, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cla: yes This human has signed the Contributor License Agreement. samples Issues that are directly related to samples.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants