WIP: Install and use source-map-support module for tests in Circle CI. #449
Conversation
| "testing" | ||
| ) | ||
|
|
||
| // Go repository basic compiler tests, and regression tests for fixed compiler bugs. | ||
| func TestGoRepositoryCompilerTests(t *testing.T) { | ||
| if b, _ := strconv.ParseBool(os.Getenv("SOURCE_MAP_SUPPORT")); os.Getenv("SOURCE_MAP_SUPPORT") != "" && !b { | ||
| t.Fatal("Source maps disabled, but required for this test. Use SOURCE_MAP_SUPPORT=true or unset it completely.") |
There was a problem hiding this comment.
I considered skip, but was worried it might be bad to simply skip such an important test so easily.
Hmm...
Maybe skip if short flag is given?
There was a problem hiding this comment.
Maybe skip if short flag is given?
I don't think that would be good, better be consistent.
I think I've seen Skip in the core packages for this situation. Reasoning might go like this:
Fail -> Implementation broken
Skip -> Environment broken/insufficient
There was a problem hiding this comment.
I can change to skip, sure, I just don't want us to get into a situation where those tests are always skipped and we don't notice/realize.
There was a problem hiding this comment.
I've looked in https://github.com/golang/go/blob/master/src/cmd/go/go_test.go to see when skip is used.
It's used to skip individual specific tests when environment is insufficient. For example, TestCgoShowsFullPathNames is skipped when cgo is not available.
The difference is that TestGoRepositoryCompilerTests runs many, many tests. Only one of those actually depends on source-map-support to be enabled. Skipping all of TestGoRepositoryCompilerTests just because source-map-support is equivalen to skipping all of cmd/go tests if cgo is not enabled. I don't think it's acceptable.
Let's find a better solution...
There was a problem hiding this comment.
In the PR description, I asked:
What do you think about requiring source-map-support for tests? Right now they're disabled in Circle CI. Enabling it fixes one failing test from Go compiler test suite (fixedbugs/issue5856.go).
Maybe it's not worth it to require source-map-support to be mandatory for tests?
There was a problem hiding this comment.
I think the most important aspect is that the CI system tests properly. Local testing has a much lower priority and I would be okay with skipping tests in that situation. If people download the source code, run the tests and they fail, then they will probably think that the tests are broken.
There was a problem hiding this comment.
I'll find an acceptable solution and update this PR; let's not merge it until then.
I've already cherry-picked the acceptable individual commits out from here.
|
LGTM, one comment. |
|
I've edited the PR description to reflect which parts of this PR have already been cherry-picked into I will get to resolving the remaining task needed to merge this sometime, but it's lower priority than #388. |
This fixes one known failing test from Go compiler test suite.
It's needed so that the source-map-support module that is installed globally will be found by Node 6.0.
I've sshed into the circle machine, and the actual path is something like: /home/ubuntu/nvm/v0.10.33/lib/node_modules Setting it explicitly seems very fragile, so I'll try this way... Maybe there's a better way?
|
I've factored out the Node update (and updated it from 6.0.0 to latest 6.2.2 version) into #484. |
|
@shurcooL What's up with this PR? |
|
The status continues to be the same as in Jun 26, 2016. Except now it's higher priority than #388. :) I still need to think about the best thing to do about source-map-support module and tests. I'll do that, and update the PR. |
|
I'm going to close this. I still haven't found a way to get this PR to a point where I'm happy with it to be merged. If I ever do that, I'll reopen. |
DO NOT MERGE. There is an outstanding task to be resolved before this is ready.
@neelance What do you think about requiring source-map-support for tests? Right now they're disabled in Circle CI. Enabling it fixes one failing test from Go compiler test suite (fixedbugs/issue5856.go).
This PR:
Updates to Go 1.6.2.Edit: This has been cherry-picked as 5fbc215.Updates to Node 6.0.Edit: Factored out into circle: Update to Node 6.2.2. #484.Contains other minor miscellaneous improvements (see commits fdad0ef and fcc469a).Edit: These have been cherry-picked as e28f4b4 and 028c56b.