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.

Sign up for GitHub

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

Jump to bottom

[internal] Amend missing timeout field in target generators #16387

Merged
merged 7 commits into from
Aug 9, 2022
Merged

[internal] Amend missing timeout field in target generators #16387

merged 7 commits into from
Aug 9, 2022

Conversation

alonsodomin
Copy link
Contributor

Adds missing timeout field to scala_junit_tests target and aligns the Shunit2 test timeout field with the same behaviour of the other backends that support test timeouts.

Documentation for each of the backends has also been updated to show the fact that now timeouts are supported for their respective target types and that they can also be applied globally.

@alonsodomin alonsodomin added the category:internal CI, fixes for not-yet-released features, etc. label Aug 3, 2022
Eric-Arellano
Eric-Arellano approved these changes Aug 3, 2022
View reviewed changes
Copy link
Contributor

@Eric-Arellano Eric-Arellano left a comment

Choose a reason for hiding this comment

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

💜

docs/markdown/Java and Scala/jvm-overview.md Outdated Show resolved Hide resolved
@Eric-Arellano
Copy link
Contributor

Oh, looks like the help message is wrong for a couple timeout fields now. For example, Shunit2TestTimeoutField says "If unset, the test will never time out.". They should probably mention the test options. PythonTestsTimeoutField is also a lie.

Maybe have the template field TestTimeoutField set help? It can access class properties to make the message dynamic, if need be. To do that, set help=lambda cls: f"My message with {cls.property}"

@alonsodomin
Copy link
Contributor Author

good catch, and thanks for the tip, templating the help message is probably best in here.

@alonsodomin
Copy link
Contributor Author

gave all the fields based on TestTimeoutField have the same help string that there wasn't specific to each of them. Also made the Python one have the exact same help string.

Eric-Arellano
Eric-Arellano reviewed Aug 3, 2022
View reviewed changes
Copy link
Contributor

@Eric-Arellano Eric-Arellano left a comment

Choose a reason for hiding this comment

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

Thank you!

docs/markdown/Go/go.md Outdated Show resolved Hide resolved
docs/markdown/Go/go.md Show resolved Hide resolved
alonsodomin and others added 4 commits August 3, 2022 18:48
@alonsodomin @Eric-Arellano
Update docs/markdown/Go/go.md
2c25598 
Co-authored-by: Eric Arellano <14852634+Eric-Arellano@users.noreply.github.com>
@alonsodomin @Eric-Arellano
Update docs/markdown/Go/go.md
5941e0d 
Co-authored-by: Eric Arellano <14852634+Eric-Arellano@users.noreply.github.com>
@alonsodomin
Remove reference to python backend in test timeouts
90f2ea2
@alonsodomin
Add blank line
c40dabb
Eric-Arellano
Eric-Arellano approved these changes Aug 3, 2022
View reviewed changes
Copy link
Contributor

@Eric-Arellano Eric-Arellano left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Woohoo! Thanks! I love this new approach :)

This was referenced Aug 9, 2022
Closed
Merged
@Eric-Arellano
Copy link
Contributor

(Feel free to merge when ready. We try to not merge other maintainers' code for them without explicit permission)

@alonsodomin alonsodomin merged commit e1f10a3 into pantsbuild:main Aug 9, 2022
@benjyw
Copy link
Sponsor Contributor

benjyw commented Aug 9, 2022

Thanks for this change!

Note for the future:

The default commit message for a squash commit is pretty awful. It just lists the first lines of all the squashed commits.

I highly recommend editing that message when you merge. Typically I delete all the cruft and paste in the original description (the first comment above).

alonsodomin added a commit that referenced this pull request Aug 10, 2022
@alonsodomin
Add support for using environment variables in JVM tests (#16455)
1ad8e45 
The implementation approach is based on the recommendations found in #15963 and, since this affects to several targets, similar as in #16387, where a base class for the field is created in core and then extended for each of the different target types.
@benjyw benjyw mentioned this pull request Aug 13, 2022
Merged
@stuhood
Copy link
Sponsor Member

stuhood commented Aug 15, 2022 •
edited
Loading

I highly recommend editing that message when you merge. Typically I delete all the cruft and paste in the original description (the first comment above).

And while doing so, try to preserve/fix the [skip-*] tags: including them can save a lot of CI time.

I suppose that we don't really have documentation for "merging a PR": only creating one, so that would likely be useful. EDIT: Opened #16519.

cczona pushed a commit to cczona/pants that referenced this pull request Sep 1, 2022
@alonsodomin @Eric-Arellano @cczona
[internal] Amend missing timeout field in target generators (pantsb…
5bb0064 
…uild#16387)

* Amend missing test timeout field and update Python docs

* Add missing overrides field to helm_unittest_tests and document test timeouts in other backends

* Fix documentation and make all test timeout fields have the same help string

* Update docs/markdown/Go/go.md

Co-authored-by: Eric Arellano <14852634+Eric-Arellano@users.noreply.github.com>

* Update docs/markdown/Go/go.md

Co-authored-by: Eric Arellano <14852634+Eric-Arellano@users.noreply.github.com>

* Remove reference to python backend in test timeouts

* Add blank line

Co-authored-by: Eric Arellano <14852634+Eric-Arellano@users.noreply.github.com>
cczona pushed a commit to cczona/pants that referenced this pull request Sep 1, 2022
@alonsodomin @cczona
Add support for using environment variables in JVM tests (pantsbuild#…
3434afd 
…16455)

The implementation approach is based on the recommendations found in pantsbuild#15963 and, since this affects to several targets, similar as in pantsbuild#16387, where a base class for the field is created in core and then extended for each of the different target types.
@huonw huonw mentioned this pull request Jul 12, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Eric-Arellano Eric-Arellano Eric-Arellano approved these changes

@stuhood stuhood Awaiting requested review from stuhood

@chrisjrn chrisjrn Awaiting requested review from chrisjrn

Assignees
No one assigned
Labels
category:internal CI, fixes for not-yet-released features, etc.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@alonsodomin @Eric-Arellano @benjyw @stuhood