Skip to content

Add the ability to run sql tests from a config file#9968

Closed
suneet-s wants to merge 1 commit intoapache:masterfrom
suneet-s:calcite-tests
Closed

Add the ability to run sql tests from a config file#9968
suneet-s wants to merge 1 commit intoapache:masterfrom
suneet-s:calcite-tests

Conversation

@suneet-s
Copy link
Contributor

@suneet-s suneet-s commented Jun 2, 2020

This PR aims to make it easier for non Druid devs to add sql tests to Druid.

If anyone in the community has particular query shapes they expect to see
working in Druid, they can add the queries to the provided config.
This will ensure the queries work as expected with each Druid release.

Sample ingestion specs are provided in the test/resources folder so that tests
are easier to build by testing against a local cluster.

yaml is chosen as the data format for the tests as it allows devs to place
comments within the configuration which helps with maintainability of the
tests.


This PR has:

  • been self-reviewed.
  • added documentation for new or modified features or behaviors.
  • added Javadocs for most classes and all non-trivial methods. Linked related entities via Javadoc links.
  • added comments explaining the "why" and the intent of the code wherever would not be obvious for an unfamiliar reader.

This PR aims to make it easier for non Druid devs to add sql tests to Druid.

If anyone in the community has particular query shapes they expect to see
working in Druid, they can add the queries to the provided config.
This will ensure the queries work as expected with each Druid release.

Sample ingestion specs are provided in the test/resources folder so that tests
are easier to build by testing against a local cluster.

yaml is chosen as the data format for the tests as it allows devs to place
comments within the configuration which helps with maintainability of the
tests.
@jihoonson
Copy link
Contributor

I like this idea 👍

@suneet-s
Copy link
Contributor Author

suneet-s commented Jun 2, 2020

Open to feedback on if this is useful / other approaches to make CalciteQueryTests more complete.

I don't think these parameterized tests need to run on every PR, having them run nightly might be a good compromise on compute time vs test coverage - but I don't know how to have that work with Travis.

I still need to try a few more query shapes to validate that it's easy to add new tests.
This framework also doesn't make it easy to deal with different results for null handling mode enabled vs disabled.

@jihoonson
Copy link
Contributor

Tagged "Design Review" because it seems not easy to change once we choose the design of the testing framework (we may need to update all the test specs in the yaml file if we want to change the test spec later).

@ccaominh
Copy link
Contributor

ccaominh commented Jun 2, 2020

I don't think these parameterized tests need to run on every PR, having them run nightly might be a good compromise on compute time vs test coverage - but I don't know how to have that work with Travis.

Travis is already setup to run a daily job, which will run any jobs that are in the cron stage. For an example, refer the the "security vulnerabilities" job: https://github.com/apache/druid/blob/master/.travis.yml#L426

The configuration that restricts the cron job to only run the cron stage is here: https://github.com/apache/druid/blob/master/.travis.yml#L41

@stale
Copy link

stale bot commented Aug 1, 2020

This pull request has been marked as stale due to 60 days of inactivity. It will be closed in 4 weeks if no further activity occurs. If you think that's incorrect or this pull request should instead be reviewed, please simply write any comment. Even if closed, you can still revive the PR at any time or discuss it on the dev@druid.apache.org list. Thank you for your contributions.

@stale stale bot added the stale label Aug 1, 2020
@stale
Copy link

stale bot commented Aug 29, 2020

This pull request/issue has been closed due to lack of activity. If you think that is incorrect, or the pull request requires review, you can revive the PR at any time.

@stale stale bot closed this Aug 29, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants