Skip to content

Conversation

@mosabua
Copy link
Member

@mosabua mosabua commented Nov 21, 2023

Description

Add guidelines for writing tests.

Additional context and related issues

Converted from google doc - follow up about role on website will come after merge.

Release notes

(x) This is not user-visible or is docs only, and no release notes are required.

However.. we should mention it somewhere quite prominently.

@cla-bot cla-bot bot added the cla-signed label Nov 21, 2023
@github-actions github-actions bot added the docs label Nov 21, 2023
@mosabua mosabua force-pushed the testguide branch 2 times, most recently from c21b59c to 45a4875 Compare November 21, 2023 23:09
@mosabua mosabua marked this pull request as ready for review December 29, 2023 00:00
@mosabua mosabua changed the title (WIP) Add test guide Add test guide Dec 29, 2023
@mosabua mosabua requested a review from martint December 29, 2023 00:18
Copy link
Member

Choose a reason for hiding this comment

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

Should we have a section about testing with SaaS, like BigQuery and Redshift? That Trino uses accounts sponsored by third parties and the credentials are not shared, so tests don't run by default in PRs, must be manually approved by maintainers and require own set of credentials to be executed locally.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think so .. but in a separate section somewhere .. maybe combined with the explanation about "product tests" or "integration tests" .. will talk to @martint to figure out what name to choose and where to put it on this page.

Copy link
Member

Choose a reason for hiding this comment

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

the problem this solves is that the test frameworks (neither JUnit not TestNG) don't allow controlling number of concurrent test methods per class.

e.g. how do you for example have a test class which mostly spends time waiting for I/O or some failure and reduce it's runtime?

Copy link
Member Author

Choose a reason for hiding this comment

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

Is this really a problem worth solving .. could you just split the test class up into multiple classes for different aspects and then you have the different classes run tests in parallel? Huge test classes with lots of test methods are probably a bad idea anyway .. any thoughts @martint ?

Copy link
Member

Choose a reason for hiding this comment

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

then you have the different classes run tests in parallel?

Still doesn't solve the problem. The concurrency of test is global in JUnit, ie. if you say 4 concurrent tests you will only have 4 no matter how many test classes you split it into.

You can only override it on a maven module basis via surefire config but that means a separate maven module (or profile) for each such "group of tests".

Copy link
Member Author

Choose a reason for hiding this comment

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

This section is still here for reference only .. I can delete any time .. imho all aspects are covered but wanted to have it here for reviewers for now

@github-actions
Copy link

This pull request has gone a while without any activity. Tagging the Trino developer relations team: @bitsondatadev @colebow @mosabua

@github-actions github-actions bot added the stale label Jan 30, 2024
@mosabua
Copy link
Member Author

mosabua commented Jan 30, 2024

Chatted with @electrum about this some more. All in your hands @martint ...

@github-actions github-actions bot removed the stale label Jan 31, 2024
@github-actions
Copy link

This pull request has gone a while without any activity. Tagging the Trino developer relations team: @bitsondatadev @colebow @mosabua

@github-actions github-actions bot added the stale label Feb 21, 2024
@mosabua
Copy link
Member Author

mosabua commented Feb 21, 2024

Reminder @martint .. I think just a quick read and merge will be okay since we plan to iterate on this anyway.

@github-actions github-actions bot removed the stale label Feb 22, 2024
@mosabua
Copy link
Member Author

mosabua commented Mar 2, 2024

All updated.. imho this is good enough for merge and later refinement @martint

@github-actions
Copy link

This pull request has gone a while without any activity. Tagging the Trino developer relations team: @bitsondatadev @colebow @mosabua

@github-actions github-actions bot added the stale label Mar 25, 2024
@mosabua
Copy link
Member Author

mosabua commented Mar 26, 2024

Waiting for final review from @martint

@github-actions github-actions bot removed the stale label Mar 27, 2024
@mosabua
Copy link
Member Author

mosabua commented Apr 3, 2024

Thank you @martint

@mosabua mosabua merged commit cbdf4e1 into trinodb:master Apr 3, 2024
@mosabua mosabua deleted the testguide branch April 3, 2024 18:09
@github-actions github-actions bot added this to the 444 milestone Apr 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Development

Successfully merging this pull request may close these issues.

5 participants