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

Integration tests for auth0 apps commands and flags [CLI-95] #278

Merged
merged 17 commits into from
May 6, 2021
Merged

Conversation

morganelle
Copy link
Contributor

@morganelle morganelle commented May 1, 2021

Description

Use Commander to add integration tests for the following commands, with tests for every flag and flag value.

  • apps create
  • apps update
  • apps show

Added two small helper scripts to store an app id for tests that require an existing app as well as a cleanup script to delete the fixtures created during testing.

Also fixed two small typos found during development.

Testing

These updates can be tested by running make integration. Verified that the apps not prefixed with integration-test- were not deleted during test cleanup.

Checklist

  • I have added documentation for new/changed functionality in this PR or in auth0.com/docs
  • All active GitHub checks for tests, formatting, and security are passing
  • The correct base branch is being used, if not master

@morganelle morganelle changed the title App tests Integration tests for auth0 apps commands and flags May 1, 2021
@morganelle morganelle changed the title Integration tests for auth0 apps commands and flags Integration tests for auth0 apps commands and flags [CLI-95] May 1, 2021
@morganelle morganelle marked this pull request as ready for review May 4, 2021 16:20
@morganelle morganelle requested review from Widcket and rene00 May 4, 2021 16:20
contains:
- NAME integration-test-nativeapp1
- DESCRIPTION NativeApp1
- TYPE Native
Copy link
Contributor

@rene00 rene00 May 5, 2021

Choose a reason for hiding this comment

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

This test is failing for me due to N being uppercase in contains but lower case in the result:

✗ [local] 'apps create type native and check output', on property 'Stdout'

Expected
[...]
  TYPE                 native
[...]
to contain

TYPE                 Native
Suggested change
- TYPE Native
- TYPE native

I can see that we're already testing this command here but this command is the plain format. Have you considered removing this plain format test all together?

Copy link
Contributor

Choose a reason for hiding this comment

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

✋ Ignore this. Error was on my side. I was running an older version of auth0.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removing the plain format tests that duplicate json format tests is a good question, though. I included it in case we want an indication when the formatting changes unexpectedly but it could be fussy to maintain.

Makefile Outdated
@@ -71,4 +71,10 @@ $(GOBIN)/auth0-cli-config-generator:

integration: $(GOBIN)/auth0-cli-config-generator $(GOBIN)/commander
auth0-cli-config-generator && commander test commander.yaml
$(MAKE) integration-cleanup
Copy link
Contributor

Choose a reason for hiding this comment

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

perhaps a nitpick here, but we could replace recursive use of make with:

run-integration:
    auth0-cli-config-generator && commander test commander.yaml
.PHONY: run-integration

integration: $(GOBIN)/auth0-cli-config-generator $(GOBIN)/commander run-integration integration-cleanup
.PHONY: integration

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated

Copy link
Contributor

@rene00 rene00 left a comment

Choose a reason for hiding this comment

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

This an awesome PR and a great step forward for auth0-cli. Nice work 💯

@rene00
Copy link
Contributor

rene00 commented May 5, 2021

Perhaps an issue on my end but when running make integration I was prompted for device flow auth:

export AUTH0_CLI_CLIENT_NAME="integration" \
    AUTH0_CLI_CLIENT_DOMAIN="REDACTED" \
    AUTH0_CLI_CLIENT_ID="REDACTED" \
    AUTH0_CLI_CLIENT_SECRET="REDACTED" \
    AUTH0_CLI_REUSE_CONFIG="false" \
    AUTH0_CLI_OVERWRITE="true"
make integration

@morganelle
Copy link
Contributor Author

morganelle commented May 5, 2021

Perhaps an issue on my end but when running make integration I was prompted for device flow auth:

export AUTH0_CLI_CLIENT_NAME="integration" \
    AUTH0_CLI_CLIENT_DOMAIN="REDACTED" \
    AUTH0_CLI_CLIENT_ID="REDACTED" \
    AUTH0_CLI_CLIENT_SECRET="REDACTED" \
    AUTH0_CLI_REUSE_CONFIG="false" \
    AUTH0_CLI_OVERWRITE="true"
make integration

@rene00 Was that before commander started executing the tests or during?

Copy link
Contributor

@Widcket Widcket left a comment

Choose a reason for hiding this comment

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

Excellent, thanks for going the extra mile with this one.

@Widcket Widcket merged commit 82eb08d into main May 6, 2021
@Widcket Widcket deleted the app-tests branch May 6, 2021 23:38
clientid=$(_jq '.ClientID')
name=$(_jq '.Name')

if [[ $name = integration-test-* ]]
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm wondering if this would only remove the apps created during the same tests execution, otherwise it might cause issues with other CI running at the same time.
thoughts?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jfatta if we have multiple integration tests on this tenant running at once, we'll want something smarter that deletes by client id. When we get CI running we'll also want to split out the test and cleanup steps so that the cleanup step is sure to run even if the tests fail.

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.

4 participants