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

Refactor testcases #45

Merged
merged 3 commits into from Mar 11, 2019

Conversation

Projects
None yet
3 participants
@achew22
Copy link
Contributor

achew22 commented Jan 7, 2019

Refactor the test cases into input/output files to make it simpler to add
and modify them.

Test cases can now be defined by creating a file inside
testdata/default (for the default test suite) or by creating a new
directory in testdata. This new directory should contain a set of
input files (*.in.md) and matching output files (*.out.md) as well
as a single options.json that defines the set of options to invoke the
markdown formatter with.

Additionally adds new flag to the test cases called "-update_goldens"
that writes the matching .out.md file if the testcase fails.

@achew22 achew22 force-pushed the achew22:testing branch from ec7d3d0 to cc16661 Jan 7, 2019

@dmitshur

This comment has been minimized.

Copy link
Member

dmitshur commented Jan 22, 2019

Thanks for the PR! I apologize, but it'll take me some time before I get a chance to review it.

I'm not sure if we should be resolving #44 by updating the test data. I think it's a higher priority to fix blackfriday to stop producing invalid output (i.e., fix russross/blackfriday#495 first).

@achew22

This comment has been minimized.

Copy link
Contributor Author

achew22 commented Jan 22, 2019

With russross/blackfriday#521 I think the regression in blackfriday is not long for this world. I'm happy to wait until then to update and then merge.

@rtfb

This comment has been minimized.

Copy link

rtfb commented Jan 24, 2019

FYI, russross/blackfriday#521 has landed.

Refactor testcases
Refactor the test cases into input/output files to make it simpler to add
and modify them.

Test cases can now be defined by creating a file inside
`testdata/default` (for the default test suite) or by creating a new
directory in `testdata`. This new directory should contain a set of
input files (`*.in.md`) and matching output files (`*.out.md`) as well
as a single `options.json` that defines the set of options to invoke the
markdown formatter with.

Additionally adds new flag to the test cases called "-update_goldens"
that writes the matching `.out.md` file if the testcase fails.

@achew22 achew22 force-pushed the achew22:testing branch from cc16661 to 34a8023 Jan 24, 2019

@achew22

This comment has been minimized.

Copy link
Contributor Author

achew22 commented Jan 24, 2019

Now that russross/blackfriday#495 is fixed, I have updated the test cases to reflect the correct behavior. @dmitshur, do you think it is now reasonable to merge this?

PS: it was a joy to update the tests after the blackfriday update go get -u ./... && go test ./... -update_goldens && go test ./... && git commit --amend && git push -f

@achew22

This comment has been minimized.

Copy link
Contributor Author

achew22 commented Feb 3, 2019

@dmitshur, friendly ping

1 similar comment
@achew22

This comment has been minimized.

Copy link
Contributor Author

achew22 commented Feb 18, 2019

@dmitshur, friendly ping

@dmitshur

This comment has been minimized.

Copy link
Member

dmitshur commented Feb 18, 2019

Hi @achew22, thanks for the pings, they are helpful.

This is on my radar, but I have some more pressing and time-sensitive work to complete first. I hope I'll be able to get to this next weekend or so.

Sorry about the delay in getting to this, I haven't had many resources available for open source maintenance lately.

@achew22

This comment has been minimized.

Copy link
Contributor Author

achew22 commented Feb 18, 2019

@dmitshur, thanks so much for replying! Is there anything I can do to lighten that load? I'm also a Googler so if you wanted to move this from your personal account under the google github user and then share the labor I would be more than happy to help.

@dmitshur

This comment has been minimized.

Copy link
Member

dmitshur commented Feb 18, 2019

Thanks for the offer to help! I will need to think more about this, and will get back to you, just after this week. :)

@achew22

This comment has been minimized.

Copy link
Contributor Author

achew22 commented Mar 7, 2019

Friendliest of pings

bring back examples, simplify, port 2 more tests
This change brings back the two examples so they continue
to show up the package documentation.

It also removes a lot of functionality that is not used and not
justified at this time, given the basic testing requirements of
markdownfmt (i.e., no need to support different options).
It simplifies the code to a bare minimum (to make future
maintenance easier) and more consistent with my other code.

It ports two more tests, TestLineBreak and TestDoubleSpacedListEnd,
to use the new file-based test suite.
@dmitshur
Copy link
Member

dmitshur left a comment

I'm going to merge this but with modifications. I'd like to simplify this as much as viable, and make it more consistent with my other code.

One of the design goals of markdownfmt has been to not have any configuration, just like gofmt. As a result, I don't anticipate needing to use options. If and when that happens, we can bring back the code that handles that, but until then, I don't want that extra unused functionality. I think the testdata directory of cmd/gofmt command is a good reference for markdownfmt to follow.

I'm also not going to keep the README inside testdata directory. I think the updateFlag convention is very common (it's used in many packages in the standard library) and it's meant for developers, not users. I prefer to minimize the surface area of the documentation I have to maintain (it becomes costly when maintaining many packages).

Thank you for working on this, and sorry again it took so long to get around to reviewing it.

I should do a better job of communicating the status of this package in the README, but for reference, I am more eager to freeze it rather than develop it further. If you're interested in changing behavior or adding features, it's better to do so on a fork.

//
// How about `this` and other stuff like *italic*, **bold** and ***super extra***.
//
}

This comment has been minimized.

@dmitshur
Done.
`
var (
updateGoldens = flag.Bool("update_goldens", false, "Set to true to update the goldens that are stored on disk.")

This comment has been minimized.

@dmitshur

dmitshur Mar 11, 2019

Member

I'll change the name of this flag slightly to be consistent with elsewhere.



Final paragraph.

This comment has been minimized.

@dmitshur

dmitshur Mar 11, 2019

Member

This extra blank line at the end wasn't there before.


aaa/あああ
----------

This comment has been minimized.

@dmitshur

dmitshur Mar 11, 2019

Member

This extra blank line at the end wasn't there before.

wantFile := filepath.Join("testdata", testMode.Name(), testName+".out.md")
want, err := ioutil.ReadFile(wantFile)
if err != nil {
t.Errorf("Unable to open input file %s.in.md: %v", in, err)

This comment has been minimized.

@dmitshur

dmitshur Mar 11, 2019

Member

s/input file/golden file/

I'm going to simplify these error messages so it won't matter, but just pointing it out.

add ".in" to escapeurl suffix, use "go vet"
escapeurl.md should've been called escapeurl.in.md. This change
fixes that.

Start using "go vet" instead of "go tool vet" in .travis.yml,
since the latter has been deprecated as of Go 1.12.
@dmitshur
Copy link
Member

dmitshur left a comment

LGTM. Thank you again!

@dmitshur dmitshur merged commit 0ad3159 into shurcooL:master Mar 11, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.
You signed in with another tab or window. Reload to refresh your session. You signed out in another tab or window. Reload to refresh your session.