Skip to content

_test.inc.js files improperly included when not building tests #655

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

Closed
flimzy opened this issue Jun 29, 2017 · 10 comments
Closed

_test.inc.js files improperly included when not building tests #655

flimzy opened this issue Jun 29, 2017 · 10 comments

Comments

@flimzy
Copy link
Member

flimzy commented Jun 29, 2017

To reproduce, create a simple package:

main.go:

package main

func main() {}

main_test.go:

package main

import "testing"

func TestFoo(t *testing.T) {}

main_test.inc.js:

/* main_test.inc.js */

then compile:

$ gopherjs build -o main.js

And observe that the output contains the contents of the inc.js file:

$ grep -n main_test main.js
2331: /* main_test.inc.js */
@dmitshur
Copy link
Member

I don't think there's a special rule about _test suffix for .inc.js files. Is this a feature request?

Is there precedent for this in Go and its support for non-.go files like .c?

@dmitshur
Copy link
Member

Is there precedent for this in Go and its support for non-.go files like .c?

Doesn't seem that way. https://golang.org/cmd/go/ says:

'Go test' recompiles each package along with any files with names matching the file pattern "*_test.go". Files whose names begin with "_" (including "_test.go") or "." are ignored.

There's nothing there about _test suffix for .c, .s, .m, .swig, etc. files:

https://golang.org/cmd/go/#hdr-File_types

@flimzy
Copy link
Member Author

flimzy commented Jun 29, 2017

It was an assumption on my part, that it should work that way. Perhaps a faulty one... I'll do some investigation.

@dmitshur
Copy link
Member

We can discuss it as a feature request and consider for inclusion if it's unavoidable. But if avoidable, we should try to avoid straying away from cmd/go mechanics.

I'll do some investigation.

👍 Let us know what you find out.

@flimzy
Copy link
Member Author

flimzy commented Jun 29, 2017

So I think I was remembering the research I did on #468, and assuming the same logic would apply to test, but that's not really a build tag, so maybe it doesn't.

So here's my attempt at making the case for supporting this:

The other (non-.go) extensions don't seem apply to the test case, since they aren't used in running tests, despite some requests to the contrary. If such a proposal is ever accepted (which seems unlikely), it's unclear whether it would even be desirable to change the behavior of _test.c (et al) files since, again, the purpose is to test CGO, not to write tests in CGO. Maybe there's a case I'm unaware of.

I think GopherJS/JavaScript may be a notable exception, since it's common to want to configure Node.js especially for running tests, in ways that don't apply to a browser. In my particular case, I was wanting to run:

global.XMLHttpRequest = require('xhr2');

when running GopherJS tests, but not when building for the browser.

Work-arounds are possible, such as:

  • Wrapping the code block in a try/catch, or other conditional that doesn't execute in a browser.
  • Creating a special 'xhr2' package that does the requisite loading in an .inc.js file, and only load that package from within tests.
  • Wrap the JS code in GopherJS boilerplate (probably fairly simple in the simple case of loading a single module)

The first two are a bit cumbersome for my taste. The latter is probably the most appealing, but could be pretty cumbersome in larger use-cases (but maybe those don't exist).

So that's my case in favor of the change.

Naturally, the counter-argument is it's non-standard, and deviates from the official cmd/go. I'm not sure it violates the spirit of the intention, but given that there are available workarounds, maybe it's best to be conservative.

@dmitshur
Copy link
Member

I want to think a little more about it before coming to a decision (and it'd be nice to hear @neelance's thoughts). But want to post my initial thoughts.

My initial reaction/feeling is that this is likely okay. Even if it deviates a little from cmd/go, it looks like the value offered by this change would be significant. Not implementing this just because it's a little out of sync with cmd/go doesn't seem like it'd be worthwhile.

Thanks for providing the rationale.

@dmitshur
Copy link
Member

dmitshur commented Jun 29, 2017

In my particular case, I was wanting to run:

global.XMLHttpRequest = require('xhr2');

when running GopherJS tests, but not when building for the browser.

About that specific change, I want to understand it better, so here's a question.

Isn't it extremely trivial to do equivalent of global.XMLHttpRequest = require('xhr2'); in Go using js package in a _test.go file?

// foo_test.go
package foo_test

func init() {
    js.Global.Set("XMLHttpRequest", js.Global.Call("require", "xhr2"))
}

Or something like that. Is this feature request because you'd prefer to write that as JavaScript in a .inc.js file out of preference?

Or is it not possible to do in a _test.go file?

@flimzy
Copy link
Member Author

flimzy commented Jun 29, 2017

I chatted with @shurcooL on Slack about this, and I think we agree this isn't the best approach for the specific problem I'm having.

The immediate solution for me was to move the code into my Go init() as:

func init() {
    js.Global.Set("XMLHttpRequest", js.Global.Call("require", "xhr2"))
    http.DefaultTransport = &http.XHRTransport{}
}

A more general solution for getting a working net/http in GopherJS might be worth also considering in another issue.

@flimzy flimzy closed this as completed Jun 29, 2017
@dmitshur
Copy link
Member

dmitshur commented Jun 29, 2017

To clarify, our discussion removed the need for this proposal to resolve a specific problem. Hence the proposal is being withdrawn/deferred. If there's a strong need for _test.inc.js that warrants making an exception and deviating from cmd/go behavior, this discussion can be had then. It's just not worth it now because there's no need right now.

@flimzy
Copy link
Member Author

flimzy commented Aug 5, 2017

I found another use-case for excluding _test.inc.js files from production builds (see go-kivik/kivik#172, and go-kivik/kivik#175 for a work-around). I'm mentioning it here as a data point in case of future discussion, but not re-opening, as a work-around wasn't terribly difficult.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants