Skip to content

[Tooling] Use universal test runner ngr everywhere #96

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

Merged
merged 8 commits into from
Oct 30, 2023
Merged

Conversation

amotl
Copy link
Member

@amotl amotl commented Oct 28, 2023

Introduction

Introduced at #64 (review), we added a universal test runner, currently located at testing/ngr.py, which is effectively just wrapping a few other calls, to be able to start maintaining a concise incantation syntax across different CI recipes.

About

This patch expands the capabilities of testing/ngr.py to be able to invoke test suites for all different kinds of example programs or environments within this repository, and updates all CI/GHA recipes to actually use it, instead of manually enumerating corresponding commands.

/cc @hlcianfagna, @hammerhead, @karynzv

@amotl amotl force-pushed the amo/ngr-everywhere branch 9 times, most recently from 9229e65 to a745cf8 Compare October 28, 2023 21:08
@amotl amotl requested review from seut and matriv October 28, 2023 21:11
@amotl amotl marked this pull request as ready for review October 28, 2023 21:13
@amotl amotl force-pushed the amo/ngr-everywhere branch from a745cf8 to f43c9f5 Compare October 28, 2023 21:14
@amotl amotl changed the title ngr everywhere [Tooling] Use universal test runner ngr everywhere Oct 29, 2023
Copy link
Contributor

@matriv matriv left a comment

Choose a reason for hiding this comment

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

Thx a lot @amotl I'm always rooting for consistency, left some comments.

@amotl amotl requested a review from matriv October 30, 2023 09:00
Copy link
Contributor

@matriv matriv left a comment

Choose a reason for hiding this comment

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

Thx, but please wait for some other reviewer, more experienced at python.

@amotl
Copy link
Member Author

amotl commented Oct 30, 2023

Thx, but please wait for some other reviewer, more experienced at python.

Thanks for the review. GH-64 would need to go in first, anyway. This PR is stacked on top of it.

@amotl amotl requested review from hammerhead and hlcianfagna and removed request for seut October 30, 2023 11:13
@amotl amotl force-pushed the amo/python-sqlalchemy branch 2 times, most recently from 7b6be1b to 7355708 Compare October 30, 2023 13:02
@amotl amotl requested review from seut and removed request for hlcianfagna October 30, 2023 13:03
Copy link
Member

@seut seut left a comment

Choose a reason for hiding this comment

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

lgtm otherwise

elif self.has_pom_xml:
run_command(f'mvn test')
elif self.has_gradle_files:
run_command("./gradlew check")
Copy link
Member

Choose a reason for hiding this comment

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

to run gradle tests, one would issue ./gradlew test. Or what do I miss?

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for spotting. I may have picked this up somewhere else.

Copy link
Member Author

@amotl amotl Oct 30, 2023

Choose a reason for hiding this comment

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

Maybe test would exclusively invoke the software tests, but check invokes both the linters, and the software tests?

Copy link
Member Author

@amotl amotl Oct 30, 2023

Choose a reason for hiding this comment

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

It looks like check is the right choice here?

The reason check is important is because it can aggregate one or more verification tasks together. This allows us to incorporate multiple tasks into a single step rather than having to execute them one by one.

The Gradle ecosystem contains a number of plugins, such as the checkstyle plugin, that provide additional functionality to the check task.

-- https://www.baeldung.com/gradle-test-vs-check

Copy link
Member

Choose a reason for hiding this comment

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

Ah ok I see, normally I'd prefer to explicit name tasks one want to run, but as a generic task command this looks feasible.

Copy link
Contributor

Choose a reason for hiding this comment

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

Keep in mind that ./gradle check doesn't work correctly for CrateDB (talking about < 5.5 versions, that use gradle`:

~/crate/crate-5.4 (5.4 ✘)⚑✭ ᐅ ./gradlew clean check                                                                                                                                                                                                                 (5.4|✭1)
Starting a Gradle Daemon (subsequent builds will be faster)
....................................................................................................................................................................................................................................................................
> Task :libs:shared:compileJava
Note: Some input files use unchecked or unsafe operations.
Note: Recompile with -Xlint:unchecked for details.
.........................................

> Task :libs:guice:compileJava
Note: Some input files use unchecked or unsafe operations.
Note: Recompile with -Xlint:unchecked for details.

> Task :libs:sql-parser:compileJava
Note: Some input files use unchecked or unsafe operations.
Note: Recompile with -Xlint:unchecked for details.

> Task :libs:shared:forbiddenApisTest FAILED

> Task :server:compileJava
Note: Some input files use or override a deprecated API.
Note: Recompile with -Xlint:deprecation for details.
Note: Some input files use unchecked or unsafe operations.
Note: Recompile with -Xlint:unchecked for details.

FAILURE: Build failed with an exception.

* What went wrong:
Execution failed for task ':libs:shared:forbiddenApisTest'.
> No signatures were added to task; use properties 'signatures', 'bundledSignatures', 'signaturesURLs', and/or 'signaturesFiles' to define those!

* Try:
> Run with --stacktrace option to get the stack trace.
> Run with --info or --debug option to get more log output.
> Run with --scan to get full insights.

* Get more help at https://help.gradle.org

BUILD FAILED in 51s
93 actionable tasks: 75 executed, 18 up-to-date

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks. ngr will need to handle CrateDB differently then. I will consider this for another iteration. Up until now, it did not need to build CrateDB yet, only auxiliary artefacts.

Copy link
Contributor

Choose a reason for hiding this comment

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

yep, it was just FYI, if you ever stumble upon this.

Base automatically changed from amo/python-sqlalchemy to main October 30, 2023 15:47
@amotl amotl force-pushed the amo/ngr-everywhere branch from f43c9f5 to f5546bf Compare October 30, 2023 15:48
@amotl amotl merged commit 0039011 into main Oct 30, 2023
@amotl amotl deleted the amo/ngr-everywhere branch October 30, 2023 15:54
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