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

Generic scala_test rule #959

Closed
wants to merge 20 commits into from

Conversation

andyscott
Copy link
Contributor

@andyscott andyscott commented Jan 27, 2020

Description

Adds a new scala_test rule that supports the SBT testing interface.

Initially I would like to have the rule merged as "experimental" or "unstable" so folks can begin using it in larger codebase and we can iterate and improve upon the implementation.

Here's some sample output where you can see ScalaTest, Scalacheck, and JUnit all running in the same target:

 bazel test //test/v2/... --sandbox_debug=true --test_output=all                                                                                      (sc:no-domain)
DEBUG: /Users/andyscott/personal/git/bazelbuild/rules_scala/scala/private/phases/phase_discover_tests.bzl:2:5: discover tests
INFO: Analyzed 3 targets (0 packages loaded, 0 targets configured).
INFO: Found 2 targets and 1 test target...
INFO: From Testing //test/v2:test:
==================== Test output for //test/v2:test:

> beginning run of com.novocode.junit.JUnitFramework
debug: Test run started
debug: Test test.v2.JUnitTest.testFoo started
debug: Test test.v2.JUnitTest.testFoo finished, took 0.001 sec
debug: Test run finished: 0 failed, 0 ignored, 1 total, 0.004s

< run of com.novocode.junit.JUnitFramework complete

> beginning run of org.scalacheck.ScalaCheckFramework
info: + TestPropertiesClass.1: OK, proved property.
info: + TestPropertiesObject.2: OK, proved property.
Passed: Total 2, Failed 0, Errors 0, Passed 2
< run of org.scalacheck.ScalaCheckFramework complete

> beginning run of org.scalatest.tools.Framework
info: TestSuiteClass:
info: - method1
info: - method2
Run completed in 97 milliseconds.
Total number of tests run: 2
Suites: completed 1, aborted 0
Tests: succeeded 2, failed 0, canceled 0, ignored 0, pending 0
All tests passed.
< run of org.scalatest.tools.Framework complete
================================================================================
INFO: Elapsed time: 2.123s, Critical Path: 2.00s
INFO: 3 processes: 2 darwin-sandbox, 1 worker.
INFO: Build completed successfully, 3 total actions
//test/v2:test                                                           PASSED in 1.5s

Executed 1 out of 1 test: 1 test passes.
INFO: Build completed successfully, 3 total actions

I'll add a detailed description once I get this working.

Note: This branch contains a lot of refactoring and cleanup that I've encountered while building the new rule. These changes have been split off into other PRs. Depending on their merge status, they may or may not be bloating the diffs here.

Motivation

Make scala_test support any testing library that implements the SBT testing interface. Ideally we can get rid of the three separate test rules and three corresponding test runners we currently have and use something generic like this.

@ittaiz
Copy link
Member

ittaiz commented Jan 27, 2020

I'll take a look once we finalize the other PRs, ok?

@andyscott andyscott mentioned this pull request Jan 31, 2020
@andyscott andyscott mentioned this pull request Feb 19, 2020
@borkaehw
Copy link
Contributor

This PR will address #951 (If I understand it correctly). Lucid is +1 for this change, we use specs2 for all Scala tests but scala_test currently doesn't support it. I notice scala_specs2_junit_test is designed for this purpose but it would be better to just have one unified scala_test. Plus, scala_junit_test and scala_test are quite similar (based on what phases are used), another good reason to support this PR.

Let's wait until all the split off PRs are merged. @ittaiz what are the PRs you are waiting on to finalize?

@ittaiz
Copy link
Member

ittaiz commented Feb 21, 2020

for one- #1005
Additionally @andyscott needs to rebase and say if this is ready for review (note the WIP header).
After that's done I'll try to get the review started within a few days.
I'll remind @andyscott that I'm waiting for his data collection over at #965 which I'd like to advance before other issues.
I'm thinking of just moving to providers without it but he said he'll collect the data in a few weeks so I'm waiting

@andyscott
Copy link
Contributor Author

I'll remind @andyscott that I'm waiting for his data collection over at #965 which I'd like to advance before other issues.
I'm thinking of just moving to providers without it but he said he'll collect the data in a few weeks so I'm waiting

It will happen :). I've been busy! I'd like to get this PR sorted out and then use any future time for reviews and shepherding through work by other folks.

@ittaiz
Copy link
Member

ittaiz commented Feb 22, 2020

It will happen :)

👍
Once we merge #1005, you rebase this PR and say that it's ready I'll see when I can make time to review it.

Just a few thoughts-
How does test discovery happen?
How does IDE integration happen?
How does test filtering happen?

@ghost
Copy link

ghost commented May 11, 2020

Status?

@ittaiz
Copy link
Member

ittaiz commented May 11, 2020

Good question. @andyscott can you rebase and answer my questions above?

@andyscott
Copy link
Contributor Author

andyscott commented May 11, 2020 via email

@darl
Copy link
Contributor

darl commented May 29, 2020

Any news?

@andyscott andyscott force-pushed the andyscott/new-test-rule branch from 3097001 to 71b3a53 Compare May 29, 2020 17:22
@andyscott
Copy link
Contributor Author

I should have this cleaned up today. Regarding your questions @ittaiz:

How does test discovery happen?

Test discovery is done by scanning the classpath and identifying classes/modules that satisfy the SBT testing interface.

How does IDE integration happen?

I'm not sure, I haven't tried it.

How does test filtering happen?

I haven't explored extending this for test filtering, so I'm not sure.

I don't have time to work on IDE integration or test filtering. I suggest we do one of two things:

  1. Merge the PR without testing IDE support and without test filtering. Other folks from the community can pick up these work items.
  2. Someone else continues on this PR to add the additional functionality before it gets merged.

I'm biased towards option 1, as the functionality is marked unstable/experimental and it gives closure to this PR.

@andyscott andyscott force-pushed the andyscott/new-test-rule branch from 71b3a53 to f112b01 Compare May 29, 2020 17:29
@ittaiz
Copy link
Member

ittaiz commented May 31, 2020

I'll take a look this weekend and see what we can do. Can you get the build to pass?

@andyscott
Copy link
Contributor Author

andyscott commented May 31, 2020 via email

@ittaiz
Copy link
Member

ittaiz commented Jun 1, 2020 via email

@andyscott andyscott changed the title WIP generic scala_test rule Generic scala_test rule Jun 1, 2020
@andyscott
Copy link
Contributor Author

We're getting closer!

if you have time, it might be good to begin reading through the code to provide feedback on organization. Separately, I am going to read through my own implementation (it's been a few months since I've dug into the Scala part of this) and add comments.

@ittaiz
Copy link
Member

ittaiz commented Jun 2, 2020 via email

gergelyfabian pushed a commit to gergelyfabian/rules_scala that referenced this pull request May 31, 2022
@liucijus
Copy link
Collaborator

closing as stale, feel free to reopen

@liucijus liucijus closed this Jan 31, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants