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

Add config file support to integration tests #2167

Conversation

pracucci
Copy link
Contributor

What this PR does:
While reviewing the PR #1878 it popped up that the PR may change the query-frontend config requirements (discussion still on-going). This made me realise that we don't have any integration test running with the config instead of CLI flags.

This PR is a first step to ease writing tests where Cortex gets started with a config file (CLI flags are still used for some overrides).

In particular:

  • Added query-frontend support to local tsdb-blocks-storage-s3 dev env
  • Introduced CortexService to have cleaner integration tests
  • Added to writeFileToSharedDir() the ability to create the path of directories
  • Enhanced query-frontend tests to run with config file too

Which issue(s) this PR fixes:
N/A

Checklist

  • Tests updated
  • Documentation added
  • CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX]

Copy link
Contributor

@pstibrany pstibrany left a comment

Choose a reason for hiding this comment

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

LGTM.

I'm little worried how many combinations we will be testing in query_frontend_test.go. Perhaps it would be worth doing some refactoring there, but it can also lead to big test obfuscation.

@pracucci
Copy link
Contributor Author

I'm little worried how many combinations we will be testing in query_frontend_test.go. Perhaps it would be worth doing some refactoring there, but it can also lead to big test obfuscation.

Me too. What're your thoughts about the refactoring? What would refactor?

@tomwilkie
Copy link
Contributor

I'm little worried how many combinations we will be testing in query_frontend_test.go. Perhaps it would be worth doing some refactoring there, but it can also lead to big test obfuscation.

I used Table testing pattern but with a generated table here: https://github.com/cortexproject/cortex/blob/master/pkg/distributor/distributor_test.go#L284

Not sure its super useful in this case, but it does allow you to ensure you hit all the combinations of different parameters.

Signed-off-by: Marco Pracucci <marco@pracucci.com>
Signed-off-by: Marco Pracucci <marco@pracucci.com>
…ectories

Signed-off-by: Marco Pracucci <marco@pracucci.com>
Signed-off-by: Marco Pracucci <marco@pracucci.com>
@pracucci pracucci force-pushed the improve-integration-tests-adding-config-file branch from 5259d1f to a66b16a Compare February 26, 2020 07:19
@pracucci
Copy link
Contributor Author

I'm little worried how many combinations we will be testing in query_frontend_test.go. Perhaps it would be worth doing some refactoring there, but it can also lead to big test obfuscation.

I used Table testing pattern but with a generated table here: https://github.com/cortexproject/cortex/blob/master/pkg/distributor/distributor_test.go#L284

Not sure its super useful in this case, but it does allow you to ensure you hit all the combinations of different parameters.

Not sure it would help much (will end up writing the same logic but in a single test). I've an idea to introduce fixtures support to the e2e framework, in order to be able to write a single test and then run it against different set of fixtures (different configs). I will give it a try when I've some spare time.

@pstibrany
Copy link
Contributor

I'm little worried how many combinations we will be testing in query_frontend_test.go. Perhaps it would be worth doing some refactoring there, but it can also lead to big test obfuscation.

Me too. What're your thoughts about the refactoring? What would refactor?

Oh, I've never replied to this. Something like what Tom linked to or what querier_test.go in Cortex does to test various combinations of encodings and chunk merging algorithms.

@pracucci
Copy link
Contributor Author

I'm little worried how many combinations we will be testing in query_frontend_test.go. Perhaps it would be worth doing some refactoring there, but it can also lead to big test obfuscation.

Me too. What're your thoughts about the refactoring? What would refactor?

Oh, I've never replied to this. Something like what Tom linked to or what querier_test.go in Cortex does to test various combinations of encodings and chunk merging algorithms.

Yes, but we need custom setup for each test (reason why there's the setup function) so I don't see a real benefit (to me it's looks like testing style: single function, like table testing, or multiple functions). However, if we introduce a generic fixture support (moving the custom setup logic to re-usable fixtures), we may actually simplify these kinds of tests and make fixtures reusable across tests, but to me it's a work decoupled from this PR.

@pstibrany
Copy link
Contributor

However, if we introduce a generic fixture support (moving the custom setup logic to re-usable fixtures), we may actually simplify these kinds of tests and make fixtures reusable across tests,

👍

but to me it's a work decoupled from this PR.

👍

@pracucci pracucci merged commit fa5b104 into cortexproject:master Feb 26, 2020
@pracucci pracucci deleted the improve-integration-tests-adding-config-file branch February 26, 2020 12:27
simonswine pushed a commit to grafana/e2e that referenced this pull request Jan 13, 2022
* Added query-frontend support to local tsdb-blocks-storage-s3 dev env

Signed-off-by: Marco Pracucci <marco@pracucci.com>

* Introduced CortexService to have cleaner integration tests

Signed-off-by: Marco Pracucci <marco@pracucci.com>

* Added to writeFileToSharedDir() the ability to create the path of directories

Signed-off-by: Marco Pracucci <marco@pracucci.com>

* Enhanced query-frontend tests to run with config file too

Signed-off-by: Marco Pracucci <marco@pracucci.com>
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