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

Reporter tests #1176

Merged
merged 3 commits into from
Jan 13, 2021
Merged

Reporter tests #1176

merged 3 commits into from
Jan 13, 2021

Conversation

SocksDevil
Copy link
Contributor

Description

Added a series of tests for the different reporters used throughout different scala versions.

I have also taken the liberty to uncomment the tests for the DiagnosticsReporter, since the toolchain flag allows for the usage of this without a development version of bazel (I think leaving them uncommented beforehand was a mistake). Do let me know what you think of that 😄

Motivation

Related to #1161 . This guarantees that different changes made to how the build is interpreted do not break error reporting.

I don't think this should close the issue, since it doesn't yet address problems in error reporting on scala 2.11 when using the DiagnosticsReporter.

@google-cla
Copy link

google-cla bot commented Jan 11, 2021

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here with @googlebot I signed it! and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers

ℹ️ Googlers: Go here for more info.

@google-cla google-cla bot added the cla: no label Jan 11, 2021
@SocksDevil
Copy link
Contributor Author

@googlebot I signed it!

@google-cla google-cla bot added cla: yes and removed cla: no labels Jan 11, 2021
@SocksDevil
Copy link
Contributor Author

I've also managed to fix the error exit code on 2.11 . Is it fine for me to use this pull request to close everything?

@liucijus
Copy link
Collaborator

please move failing tests under test_expect_failure, any other tests should go under test

@liucijus
Copy link
Collaborator

I've also managed to fix the error exit code on 2.11 . Is it fine for me to use this pull request to close everything?

If this fix does not overlap/depend on this PR, I prefer a smaller separate PR.

@liucijus
Copy link
Collaborator

please move failing tests under test_expect_failure, any other tests should go under test

Because these test are version based, maybe they can be moved to a package under version tests, and be excluded in version test case to avoid failure, WDYT @andrefmrocha?

@SocksDevil
Copy link
Contributor Author

I've gone ahead and made the changes in question. I've decided to keep the package outside since the version_specific_tests_dir but still inside the test_version since, even though I do believe there is a flag so that a specific package is not built with commands such as bazel build //..., I think this would make the tests quite more complicated to read.

Do let me know what you think!

Copy link
Collaborator

@liucijus liucijus left a comment

Choose a reason for hiding this comment

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

LGTM!

Copy link
Member

@ittaiz ittaiz left a comment

Choose a reason for hiding this comment

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

Thanks!

@liucijus liucijus merged commit 5df8033 into bazelbuild:master Jan 13, 2021
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.

3 participants