Skip to content
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

Remove use of gotestsum in Makefile #706

Conversation

cuminandpaprika
Copy link
Contributor

Fixes an issue with make test failing when gotestsum is installed by removing gotestsum from the Makefile.

Closes #705

@anz-bank/sysl-developers

@codecov
Copy link

codecov bot commented Mar 18, 2020

Codecov Report

Merging #706 into master will not change coverage by %.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #706   +/-   ##
=======================================
  Coverage   84.05%   84.05%           
=======================================
  Files          70       70           
  Lines        9490     9490           
=======================================
  Hits         7977     7977           
  Misses       1215     1215           
  Partials      298      298           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3ac17a2...8ef12b4. Read the comment docs.

@ghost
Copy link

ghost commented Mar 18, 2020

Hang on. go test -json ./... is failing, not gotestsum. That means something is actually wrong with the test infrastructure (or a particular test). Removing gotestsum will just hide that problem. This is absolutely the wrong way to "fix" the issue

@cuminandpaprika
Copy link
Contributor Author

Hang on. go test -json ./... is failing, not gotestsum. That means something is actually wrong with the test infrastructure (or a particular test). Removing gotestsum will just hide that problem. This is absolutely the wrong way to "fix" the issue

That's quite true, it doesn't resolve the root cause of the issue, but it does resolve the fact that running make test fails.

Personally, I'd prefer not to have gotestsum rather than have the makefile not work as expected on a branch.

Given that, I'll give fixing that test a try because it does seem more beneficial

@cuminandpaprika
Copy link
Contributor Author

@ericzhang6222 Could I please get some help with 849153c#r37910456

@cuminandpaprika
Copy link
Contributor Author

Okay, after spending far too long messing around with the guts of

  • logrus
  • test2json
  • kingpin
    Here's what I've established:
  • Running go test --json github.com/anz-bank/sysl/cmd/sysl -run TestMain3 produces a failing test.
  • This is due to the fact that test2json reads from the output of go test, and parses tokens such as --- PASS:
  • The line that's causing the test to fail is L916 of sysl_test.go
  • This is because running the test results in the === PASS to not be printed correctly.

Passing output should look like

=== RUN   TestMain3
--- PASS: TestMain3 (0.00s)
PASS
ok      github.com/anz-bank/sysl/cmd/sysl       0.353s

Rather than

go test -timeout 30s -v github.com/anz-bank/sysl/cmd/sysl -count=1 -run TestMain3
=== RUN   TestMain3
usage: sysl [<flags>] <command> [<args> ...]

System Modelling Language Toolkit

Optional flags:
      --help        Show context-sensitive help (also try --help-long and
                    --help-man).
      --version     Show application version.
      --log="warn"  log level: [debug,info,warn,trace,off]
  -v, --verbose     enable verbose logging
      --root=ROOT   sysl root directory for input model file. If root is not
                    found, the module directory becomes the root, but the module
                    can not import with absolute paths (or imports must be
                    relative).

Commands:
  help [<command>...]
    Show help.

  codegen --transform=TRANSFORM --grammar=GRAMMAR [<flags>] <MODULE>...
    Generate code

  datamodel [<flags>] <MODULE>...
    Generate data models

  env
    Print sysl environment information.

  export [<flags>] <MODULE>...
    Export sysl to external types. Supported types: Swagger

  generate-db-scripts [<flags>] <MODULE>...
    Generate db script

  generate-db-scripts-delta [<flags>] <MODULE>...
    Generate delta db scripts

  import --input=INPUT --app-name=APP-NAME [<flags>]
    Import foreign type to sysl. Supported types: [grammar, openapi, swagger,
    xsd]

  info
    Show binary information

  integrations [<flags>] <MODULE>...
    Generate integrations

  mod init [<name>]
    initializes and writes a new go.mod to the current directory

  protobuf [<flags>] <MODULE>...
    Generate textpb/json

  repl
    Enter a sysl REPL

  sd [<flags>] <MODULE>...
    Generate Sequence Diagram

  template --template=TEMPLATE --start=START [<flags>] <MODULE>...
    Apply a model to a template for custom text output

  test-rig --template=TEMPLATE [<flags>] <MODULE>...
    Generate test rig

  ui [<flags>] <MODULE>...
    Starts the Sysl UI which displays a visual view of Apps defined in the sysl
    model.

  validate <MODULE>...
    Validate the sysl file


ok      github.com/anz-bank/sysl/cmd/sysl       0.356s

The next piece of the puzzle is this reported issue where redirecting os.Stdout causes tests to fail
golang/go#37304

A search in the kingpin repo reveals that os.
https://github.com/alecthomas/kingpin/search?q=stdout&unscoped_q=stdout

Summary

We can't change this test to pass without some serious rework (as far as I can see) so we're left with 2 options:

  • Remove the assert call
  • Change the Makefile to run without gotestsum

The best option still seems to be to change the Makefile so that make test works 100% of the time, and accept that gotestsum doesn't work on this repo, for now.

@cuminandpaprika
Copy link
Contributor Author

The failing of tests in gotestsum is resolved in go1.4. Closing this PR

anzopensource pushed a commit that referenced this pull request Jul 4, 2022
* Remove app-name and import-paths as required flags

* Make app-name required for certain importers

* Replace `With` functions with Configure

* Fix default import, add more tests
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

make test fails when gotestsum is installed
1 participant