Added ability to configure reporter class #1112

viclovsky · 2020-09-22T15:52:52Z

googlebot · 2020-09-22T15:52:57Z

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

It's possible we don't have your GitHub username or you're using a different email address on your commit. Check your existing CLA data and verify that your email is set on your git commits.

Corporate signers

Your company has a Point of Contact who decides which employees are authorized to participate. Ask your POC to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the Google project maintainer to go/cla#troubleshoot (Public version).
The email used to register you as an authorized contributor must be the email used for the Git commit. Check your existing CLA data and verify that your email is set on your git commits.
The email used to register you as an authorized contributor must also be attached to your GitHub account.

ℹ️ Googlers: Go here for more info.

viclovsky · 2020-09-22T19:13:37Z

@googlebot I signed it!

googlebot · 2020-09-22T19:13:52Z

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

ittaiz

Thanks!
This change looks good in general.
I think there's value in adding a test that demonstrates this ability and more importantly covers regressions.
I'd hate for someone to change this in 3 months only to find out that twitter is broken when you try to update internally.
I know this makes this PR more complicated but I think the value of always covering features with tests is huge.

ittaiz · 2020-09-28T18:09:09Z

@viclovsky I don't think the test will break if I stop using the attribute and use the default value. Maybe have the custom reporter write to a custom file and then add an e2e test (bash) that checks this file with something like "hi from custom"?

viclovsky · 2020-09-29T16:57:12Z

@viclovsky I don't think the test will break if I stop using the attribute and use the default value. Maybe have the custom reporter write to a custom file and then add an e2e test (bash) that checks this file with something like "hi from custom"?

@ittaiz thanks a lot for the comment.
I faced with problem that e2e test (Bash script) can't get the created custom file from sandbox. Do you have any idea how to do it?

I can write e2e test which checks output that Custom Reporter generates into stdout. But it would be great to find a way to do with file.

ittaiz · 2020-09-29T17:06:49Z

Maybe by writing to TEST_UNDECLARED_OUTPUTS_DIR? https://docs.bazel.build/versions/master/test-encyclopedia.html If I remember correctly it's available after execution

…

On Tue, Sep 29, 2020 at 7:57 PM Victor Orlovsky ***@***.***> wrote: @viclovsky <https://github.com/viclovsky> I don't think the test will break if I stop using the attribute and use the default value. Maybe have the custom reporter write to a custom file and then add an e2e test (bash) that checks this file with something like "hi from custom"? @ittaiz <https://github.com/ittaiz> thanks a lot for the comment. I faced with problem that e2e test (Bash script) can't get the created custom file from sandbox. Do you have any idea how to do it? I can write e2e test which checks output that Custom Reporter generates into stdout. But it would be great to find a way to do with file. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1112 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAKQQF4OBTAOCQ6EKK5RZNDSIIGXRANCNFSM4RV6JEYA> .

-- *Ittai Zeidman* 40 Hanamal street, Tel Aviv, Israel <http://www.wix.com>

ittaiz

Looks good.
Q- did you see the test fail and for the right reason? (for example by still using a hardcoded value)? We've had many times where bash tests didn't fail or worse, failed for the wrong reasons.

viclovsky · 2020-09-29T21:02:37Z

Looks good.
Q- did you see the test fail and for the right reason? (for example by still using a hardcoded value)? We've had many times where bash tests didn't fail or worse, failed for the wrong reasons.

Yes, I've checked -- e2e test (bash script) fails on changing output file or even on removing reporter_class parameter from scala_test. The error looks like
File /private/var/tmp/_bazel_vicdev/1f71015f51ed51b69665f449a86c83b2/execroot/io_bazel_rules_scala/bazel-out/darwin-fastbuild/testlogs/test/scala_test/custom_reporter/test.outputs/custom_reporter_check does not exist.

ittaiz · 2020-09-29T21:07:59Z

👍🏽 Linting fails. Please run lint.sh with fix

ittaiz · 2020-09-30T17:02:07Z

restarting travis

or-shachar · 2020-10-01T09:24:33Z

scala/private/rules/scala_test.bzl

    "colors": attr.bool(default = True),
    "full_stacktraces": attr.bool(default = True),
    "jvm_flags": attr.string_list(),
+    "reporter_class": attr.string(


Looks amazing!

Sorry for post merge CR but I have a small comment!

I'd use Bazel's doc attribute to document this new attribute.
I'm guessing one day we would want to auto-generate full manual and I think reverse engineering and by git blame is not the best way to go.

* Added ability to configure reporter class * Added test for "reporter_class" option * Added e2e test (bash script) which checks output file from Custom report * Added test_custom_reporter_class.sh to test_rules_scala.sh * Fix lint

viclovsky requested a review from ittaiz as a code owner September 22, 2020 15:52

googlebot added the cla: no label Sep 22, 2020

Added ability to configure reporter class

28e744f

viclovsky force-pushed the custom_reporter branch from 2bd427b to 28e744f Compare September 22, 2020 15:57

googlebot added cla: yes and removed cla: no labels Sep 22, 2020

liucijus approved these changes Sep 23, 2020

View reviewed changes

ittaiz reviewed Sep 25, 2020

View reviewed changes

Added test for "reporter_class" option

28a0346

viclovsky requested a review from ittaiz September 27, 2020 22:26

viclovsky added 2 commits September 29, 2020 21:14

Added e2e test (bash script) which checks output file from Custom report

fd8e8d9

Added test_custom_reporter_class.sh to test_rules_scala.sh

2ecd5c1

ittaiz approved these changes Sep 29, 2020

View reviewed changes

Fix lint

cc8bbf5

ittaiz closed this Sep 30, 2020

ittaiz reopened this Sep 30, 2020

ittaiz merged commit 725b004 into bazel-contrib:master Sep 30, 2020

or-shachar reviewed Oct 1, 2020

View reviewed changes

Uh oh!

Added ability to configure reporter class #1112

Added ability to configure reporter class #1112

Uh oh!

Conversation

viclovsky commented Sep 22, 2020

Uh oh!

googlebot commented Sep 22, 2020

What to do if you already signed the CLA

Individual signers

Corporate signers

Uh oh!

viclovsky commented Sep 22, 2020

Uh oh!

googlebot commented Sep 22, 2020

Uh oh!

ittaiz left a comment

Choose a reason for hiding this comment

Uh oh!

ittaiz commented Sep 28, 2020

Uh oh!

viclovsky commented Sep 29, 2020

Uh oh!

ittaiz commented Sep 29, 2020 via email

Uh oh!

ittaiz left a comment

Choose a reason for hiding this comment

Uh oh!

viclovsky commented Sep 29, 2020

Uh oh!

ittaiz commented Sep 29, 2020 via email • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ittaiz commented Sep 30, 2020

Uh oh!

or-shachar Oct 1, 2020

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

ittaiz commented Sep 29, 2020 via email •

edited

Loading