Fixup some of the tests #219
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
In test_run.sh: * Move test_build_identical to the end (as per bazelbuild#217), add comment * Move all the binary runs together, sort them In test/BUILD: * Add missing test binary runs (and sort them) Now is same as in test_run.sh
|
👍 |
|
So there's good news and bad news. 👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there. 😕 The bad news is that it appears that one or more commits were authored by someone other than the pull request submitter. We need to confirm that they're okay with their commits being contributed to this project. Please have them confirm that here in the pull request. Note to project maintainer: This is a terminal state, meaning the |
|
I'm okay with my commits (the merge conflict resolve) being included cibot. |
|
Why do we need to add all the binaries in the test/BUILD? What confidence does this bring us? |
|
They were originally added to ensure there was no discrepancy between blaze
test using the binary and blaze run due to runfiles weirdness.
At this point though (with runfiles setup being sorted by the launcher), I
put them in test/BUILD because I'm more likely to do a quick blaze test
test/... than run test_run.sh when in the middle of changes.
…On Jun 5, 2017 9:57 PM, "Ittai Zeidman" ***@***.***> wrote:
Why do we need to add all the binaries in the test/BUILD? What confidence
does this bring us?
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#219 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AA9_-OJNDx3OaigLPdKx0oFmVMu4S6m4ks5sBNxNgaJpZM4Nwn_Q>
.
|
|
I see. Then maybe we should cut down the number of happy cases we have in
test_run.sh? We obviously need a few for confidence and regression of
"bazel run" but I definitely agree the feedback cycle of "bazel test
//test/..." is much faster
…On Tue, 6 Jun 2017 at 8:00 Stephen Twigg ***@***.***> wrote:
They were originally added to ensure there was no discrepancy between blaze
test using the binary and blaze run due to runfiles weirdness.
At this point though (with runfiles setup being sorted by the launcher), I
put them in test/BUILD because I'm more likely to do a quick blaze test
test/... than run test_run.sh when in the middle of changes.
On Jun 5, 2017 9:57 PM, "Ittai Zeidman" ***@***.***> wrote:
> Why do we need to add all the binaries in the test/BUILD? What confidence
> does this bring us?
>
> —
> You are receiving this because you authored the thread.
> Reply to this email directly, view it on GitHub
> <
#219 (comment)
>,
> or mute the thread
> <
https://github.com/notifications/unsubscribe-auth/AA9_-OJNDx3OaigLPdKx0oFmVMu4S6m4ks5sBNxNgaJpZM4Nwn_Q
>
> .
>
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#219 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ABUIFyRmIWtUNGxKaRP6DUsZhE62UhTsks5sBNztgaJpZM4Nwn_Q>
.
|
|
Sounds reasonable.
On Mon, Jun 5, 2017 at 10:08 PM, Ittai Zeidman <notifications@github.com>
wrote:
… I see. Then maybe we should cut down the number of happy cases we have in
test_run.sh? We obviously need a few for confidence and regression of
"bazel run" but I definitely agree the feedback cycle of "bazel test
//test/..." is much faster
On Tue, 6 Jun 2017 at 8:00 Stephen Twigg ***@***.***> wrote:
> They were originally added to ensure there was no discrepancy between
blaze
> test using the binary and blaze run due to runfiles weirdness.
>
> At this point though (with runfiles setup being sorted by the launcher),
I
> put them in test/BUILD because I'm more likely to do a quick blaze test
> test/... than run test_run.sh when in the middle of changes.
>
> On Jun 5, 2017 9:57 PM, "Ittai Zeidman" ***@***.***>
wrote:
>
> > Why do we need to add all the binaries in the test/BUILD? What
confidence
> > does this bring us?
> >
> > —
> > You are receiving this because you authored the thread.
> > Reply to this email directly, view it on GitHub
> > <
> #219#
issuecomment-306382079
> >,
> > or mute the thread
> > <
> https://github.com/notifications/unsubscribe-auth/AA9_-
OJNDx3OaigLPdKx0oFmVMu4S6m4ks5sBNxNgaJpZM4Nwn_Q
> >
> > .
> >
>
> —
> You are receiving this because you commented.
>
>
> Reply to this email directly, view it on GitHub
> <https://github.com/bazelbuild/rules_scala/pull/
219#issuecomment-306382353>,
> or mute the thread
> <https://github.com/notifications/unsubscribe-auth/
ABUIFyRmIWtUNGxKaRP6DUsZhE62UhTsks5sBNztgaJpZM4Nwn_Q>
> .
>
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#219 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AA9_-J1zq5VgxKsOeSb06pv-3_aS753uks5sBN62gaJpZM4Nwn_Q>
.
|
Closed
|
Thinking more about this, I'm not sure it does matter where the build is identical test goes in the order. Yes it removes everything but it also rebuilds everything. Why does it matter where it goes in order? |
|
I think you're right. We can also in the build logs that the tests which
follow take a short time so it shouldn't be an issue.
…On Sun, 11 Jun 2017 at 20:37 P. Oscar Boykin ***@***.***> wrote:
Thinking more about this, I'm not sure it does matter where the build is
identical test goes in the order. Yes it removes everything but it also
rebuilds everything.
Why does it matter where it goes in order?
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#219 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ABUIFw65XzrkqBDCZf8drC54ZzBOK9r8ks5sDCXPgaJpZM4Nwn_Q>
.
|
|
I re-reviewed. For some reason, I earlier thought that the identical test was only checking some of the jars under test/..., but it seems to be checking all of them. This is far less useful now. It is maybe nice to do the identical test at the end because it is the longest so if you break a test after it in test_run, you aren't waiting as long. |
|
Yeah, that's true. For that reason running last is a good idea.
…On Mon, Jun 12, 2017 at 14:16 Stephen Twigg ***@***.***> wrote:
I re-reviewed. For some reason, I earlier thought that the identical test
was only checking some of the jars under test/..., but it seems to be
checking all of them.
This is far less useful now. It is maybe nice to do the identical test at
the end because it is the longest so if you break a test after it in
test_run, you aren't waiting as long.
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#219 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAEJdr4WSPR9DvLgk7mDosM__iYGH2Yhks5sDdTNgaJpZM4Nwn_Q>
.
|
|
I suggest, rebase, green build and let's merge |
|
Just got back from a small vacation. Will try to hit both over the weekend (as catching up on my directwork related tasks must take priority :) ) |
|
:)
Awesome! This is starting to be a blocker for us since we need *runtime*
location expansion and that is only supported (partially) in java_test.
This means I need to switch test running to be a macro (scala library and
java_test). If there is anything I can do to help please let me know.
…On Fri, 23 Jun 2017 at 8:52 Stephen Twigg ***@***.***> wrote:
Just got back from a small vacation. Will try to hit both over the weekend
(as catching up on my directwork related tasks must take priority :) )
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#219 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ABUIF1vLPph9-cIiRSAubV1oxqnWUhaRks5sG1KugaJpZM4Nwn_Q>
.
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
In test_run.sh:
In test/BUILD:
Now is same as in test_run.sh