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

Creating CLI e2e tests #2494

Merged
merged 18 commits into from
May 12, 2023
Merged

Creating CLI e2e tests #2494

merged 18 commits into from
May 12, 2023

Conversation

danielbdias
Copy link
Contributor

@danielbdias danielbdias commented May 5, 2023

This PR adds a PoC for CLI e2e tests on Tracetest.

To run these tests locally, you can run the following commands:

cd testing/cli-e2etest
make test

Or, if you want to run with your local compilation:

make build-docker
rm -rf dist
make build-go

export TRACETEST_COMMAND=$PWD/dist/tracetest
export TEST_ENVIRONMENT=jaeger
export TAG=latest

cd testing/cli-e2etest
make test

Loom: https://www.loom.com/share/92fe592a94614b57a85fd0715f1c9293

Changes

  • Added CLI e2e action on Github Actions
  • Added test code for DataStores, version and help commands

Fixes

  • Found a wrong behavior with apply command, that was not considering yaml when parsing a datastore

Checklist

  • tested locally
  • added new dependencies
  • updated the docs
  • added a test

@danielbdias danielbdias changed the title Moving CLI and Server external tests to 'testing' folder Creating CLI e2e tests May 8, 2023
@danielbdias danielbdias marked this pull request as ready for review May 10, 2023 19:50
Comment on lines +87 to +88
request.Header.Set("Content-Type", contentType)
request.Header.Set("Accept", contentType)
Copy link
Member

Choose a reason for hiding this comment

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

Is that correct? We expect the same value for Content-Type and Accept? Doesn't this go against the PR Sebastian opened to support different content types for inputs and outputs of the API?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, because with that we are saying "we expect the same input and output contentType from the API", on future versions we have the idea of sending yaml and handling the output with the --output-format flag that the user uses.

Copy link
Collaborator

Choose a reason for hiding this comment

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

for future referece, if you don't need different request and response content types, you can pass just one (either) header and everything falls back to that

Copy link
Contributor Author

@danielbdias danielbdias May 12, 2023

Choose a reason for hiding this comment

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

Cool! I will revisit this decision and start using different Content-Type and Accept after we visit the PR #2523 .
In the future, the main idea is that the backend should be responsible for formatting entities to avoid replication of the same logic and both CLI and Backend.

@danielbdias danielbdias merged commit cb5e526 into main May 12, 2023
32 checks passed
@danielbdias danielbdias deleted the add/cli-e2e-tests branch May 12, 2023 14:25
schoren pushed a commit that referenced this pull request Jun 5, 2023
* Moving CLI and Server external tests to 'testing' folder

* wip - First draft of e2e test

* Structuring tests

* Adding cli e2e tests on CI

* Fixing e2e tests

* Updating CI command

* Improved internal CLI e2e structure

* Update Github action script

* Fixing runtime

* Adding tests and fixing error on CLI

* Fixing apply command

* Fixing test behavior

* Fixing test

* Adding command to avoid test cache

* Fix datastore to consider new behavior

* Updated tests
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.

None yet

3 participants