Update protobuf tests to use forked Closure test suite #636
Conversation
mollyibot
requested review from
soldair,
gregmagolan,
gkdn and
jDramaix
as code owners
|
There are some unrelated changes in your PR. |
| @@ -20,5 +20,5 @@ goog.require('io.bazel.rules.closure.protobuf.Example'); | |||
| function testExample() { | |||
| var msg = new io.bazel.rules.closure.protobuf.Example('value'); | |||
| msg.field(); | |||
There was a problem hiding this comment.
Does this mean that this whole test should be dropped?
There was a problem hiding this comment.
just the line; it doesn't do anything.
without those change, the checks would fail like this one https://buildkite.com/bazel/rules-closure-closure-compiler/builds/2960 These changes are to make the build pass. |
If you are trying to fix existing build, it should go to its own PR. Your PR and title would be otherwise very misleading. BTW, you also dropped the tests in the last commit. Was that intentional? |
Added the test back and removed unrelated change. |
|
I'm confused. Have you intentionally reverted changes to closure/protobuf/test/legacy/BUILD? |
nice catch, the file was local but missed in the commit. Added it back. |
|
"Update protobuf test to use jssuite from forked files" -> "Update protobuf tests to use forked Closure test suite" Pls also double check your PR commit message to make sure it is clean. |
@mollyibot I think you missed my reminder. You need to clean up commit messages while merging GitHub PRs. |
mollyibot
changed the title
sorry I missed this before merging the pr. I will clean up commit messages for next pr. |
No description provided.