Allow building and testing the "go-jsonnet" project with Bazel #281
Conversation
|
I see that a few tests failed in the Travis CI build, but they pass for me locally: What's different about the Travis CI testing environment? |
|
I fixed the c-bindings tests; they were missing the standard library AST being in place. The problem with the file paths in the normal Jsonnet tests may be due to a difference between macOS and Linux. I'll probably have to wait until Tuesday to explore the differences on another computer. |
main_test.go
Outdated
| @@ -402,22 +412,42 @@ func expandEscapedFilenames(t *testing.T) error { | |||
| return string([]byte{byte(code)}) | |||
| }) | |||
| if file != input { | |||
| if err := os.Link(input, file); err != nil { | |||
| link := filepath.Join(dstDir, file[9:]) // Strip "testdata/" prefix. | |||
There was a problem hiding this comment.
This part was already quite hacky and now it seems way too complicated for what it does (we are testing imports with weird filenames, which we can't even keep in the repo, because that causes trouble on Windows). Perhaps we can make it simpler by always making a symbolic link or even a copy of the file instead?
There was a problem hiding this comment.
Funny, I did start writing a version of this that copies the files, and bumped into that unusual lack of a "copy file" procedure in the Go standard library. I went off reading about the various ways to copy files, shook my head, then went back to making the links work.
Since these files are all rather small, copying the files "the easy way" (ioutil.ReadFile to ioutil.WriteFile) should work well enough.
That wasn't the only challenge, though. The golden output files with error messages inside contain an expected file path. Making the actual output match that closely enough required yet more munging.
|
Thank you! I haven't read it very carefully yet, but so far it looks good to me.
What is the problem? |
It looks to me like the tests running on Linux can't find the symbolic links created for the import_various_literals_escaped.jsonnet and importstr_various_literals_escaped.jsonnet files. It's hard to decipher the error messages as they show up in the Travis CI view, because they're color-coded diffs that are only easy enough to tease apart when you have a very clear idea of what each side says. I hope to experiment with this more this afternoon, with access to a computer running Linux. |
|
I spent a while on a computer running Linux trying different approaches to getting the tests that involve linked files to work. I'm down to this problem: In import_various_literals_escaped.jsonnet, there are four However, since the linked ".jsonnet file is sitting in a different directory, the error message printed by the interpreter contains that absolute path: In other cases I've been able to adjust the "name" of the test case to get this file path mentioned in the error message to match the path in the "golden" file, but those cases are where the error message is mentioning the top-level file path. Here, the error message is mentioning an imported path, not the top-level path. I could scrub the resulting error message, mapping the temporary test directory to the relative testdata path if the test fails with an error and the output has the temporary test directory path as its prefix. That seems rather hacky. Can you think of any other approaches to try? Do we need these escaped file names because the repository can't even be checked out on a computer running Windows? I see that we skip linking these files when running on Windows, and don't try to read the percent-named files. We could avoid a lot of trouble here if we could get away with at least storing the files with the intended names in the repository. |
It happened in this change. Testing this stuff is a massive time sink. Maybe at this point we should just get rid of it. Later perhaps we can add a unit test or something. It someone has a What do you think? |
|
First, I appreciate you recognizing the difficulty of making these tests work. I still haven't figured out why they work on macOS, but don't work on Linux. Knowing what I know now, I'd expect them to fail in both environments. I think it may be possible to test these special cases, but not as we're doing it now with these encoded file names. Instead we may be able to dedicate a separate test function for them, where we're not reading Jsonnet/golden file pairs from the repository, but where we instead write out Jsonnet code defined in the program source to the oddly-named files and compare the evaluation result to "golden" expectations also defined in the program source. That would avoid the current awkward need to leave some existing files in place, but write either links or copies into a different directory, which makes the paths that appear in the evaluation errors more difficult to predict. It would help if you could clarify your note that the expected failure in the %22.golden file may be wrong. Does the content of that file need to change? Was its change in c345915 a mistake? |
|
Ah, actually it wasn't c345915. I don't know what I saw yesterday, but right now it seems like this test was slightly broken from the beginning. It was never intended to fail with "unexpected end of file". I agree that what you described would be a better approach to testing these things. For now I've just fixed the error in #283, so that there is no stack trace and no problematic paths in the output. That should solve our problem here. |
|
@sbarzowski, please take another look. I separated the unit test cases for the special filenames (".jsonnet and '.jsonnet) into a dedicated test (called Travis CI appears to be happy with it, and as far as I can prove here, it builds with both go build and bazel build, and the tests run successfully with both go test and bazel test. |
|
One annoyance: If you use go build to build the c-bindings/c-bindings program, cgo generates the c-bindings/libgojsonnet.h file in that directory. If you then run bazel run //:gazelle, Gazelle will add this libgojsonnet.h file to the "go_default_library" target's "srcs" list in the c-bindings/BUILD.bazel file, because Gazelle found that file now sitting in that directory. Say that you commit that change to the BUILD.bazel file. It now references a generated file—one nominated in c-bindings/.gitignore—that won't be there in a fresh checkout. The next time you run bazel build //c-bindings, it will fail, because the BUILD.bazel file says that c-bindings/libgojsonnet.h is a required dependency, but it doesn't exist; it won't exist in that directory until we run go build again. I looked for a way to tell Gazelle to not add a particular file to the "srcs" attribute there. The best we could do is to annotate the entire "srcs" list with a |
| for _, f := range []string{"true"} { | ||
| for _, ext := range []string{".jsonnet"} { | ||
| name := f + ext | ||
| copySmallFile(t, filepath.Join(dir, name), filepath.Join("testdata", name)) |
There was a problem hiding this comment.
Note that I considered trying to run these evaluation tests using just in-memory content, avoiding the filesystem, but two things motivated doing it with files instead:
- We can reuse the
runTestfunction, which assumes that it will use files as input. - We'd have to rig up our own
Importerto handle the test cases where the top-level input file imports another one of these files.
Writing just the true.jsonnet file probably would have been sufficient to cover this need, but the first motivation argued against duplicating or factoring the common parts from therunTestsfunction.
Let me know if you'd prefer that we just process in-memory content here.
|
I like the changes. I tried on my machine and it seems to be working nicely. Though at some point, when I used bazel-compiled Jsonnet with cpp-jsonnet tests, I had two bizarre test failures which disappeared when I tried to debug them and I was unable to reproduce them from a clean state, so probably it was something with my setup. Later we'll need to test bazel-built Jsonnet in CI, but I feel like this change is already big enough. @sparkprime Do you want to take a look, or should I just merge this? |
|
@sparkprime, do you have any feedback? Can we proceed with merging? |
Export all fields in struct types in the "ast" package to allow generating program source to reconstruct their complete values in a separate package.
Write the files with verboten names (within some operating systems) to a temporary directory, in order to avoid committing them to the VCS repository.
|
I had a quick read and LGTM. Thanks for all the unrelated yak shaving you did here! |
|
Hmmm... it looks unrelated to this change, but after merging this we got a failure when running CI on master on Go tip. It fails when running |
|
gocov isn't mentioned in the go.mod or go.sum file, but I see that we pull it with go get here in .travis.yml. Searching for that error message leads only back to the source file that emits it. I don't find other people complaining about it (yet). |
|
I'm unable to reproduce the error locally on macOS using Go version 1.12.6. |
|
Given that gimme is using the current tip version, I wonder if golang/go@4c2ffd2 may be related. It's the only recent commit I see there that sounds like it could be involved. |
|
See bazel-contrib/rules_jsonnet#113 for a proposed use of these Bazel targets. |
In order to more easily facilitate use of the Go port of the jsonnet tool—instead of the C++ port—from consuming projects like rules_jsonnet, allow this project to be built with Bazel.
There are a few concessions introduced here:
astgen)Doing so breaks a circular dependency otherwise caused by generating this source file into the
astpackage.astpackageWriting the dumped struct initializers into the
astgenpackage requires being able to mention the struct fields defined in theastpackage from a separate package (astgen).astpackage explicitlyThe AST generated in the
astgenpackage is made available to programs that import it, but it doesn't automatically set this AST as the one used by theastpackage. Doing so is possible in aninitfunction in the generated in theastgenpackage, but setting it up automatically there felt too mysterious. Instead, do it explicitly where necessary: in the jsonnet tool and in the test package that uses it.When running the program with Bazel, both its input file path and its output file path differ from the seemingly more obvious paths used when invoking it outside of Bazel.
When building with Bazel, the include file paths are different from the paths used when building outside of Bazel. Use the cgo directives to accommodate both environments.