~eliasnaur/gio#173: 
Refactor cmd/gogio flags to avoid globals

Global flags need to be avoided in order to add some tests, I will look into this sometime in the upcoming weeks.

After that, we can revisit this commit:

9bbeb92b cmd/gogio: reuse the test binary to call gogio in the e2e tests

It was created by ~mvdan, where he wrote:

An alternative here would have been to refactor main.go to allow being called directly. However, that would have required a non-trivial refactor, since flag parsing is done via globals. Given that the TestMain method is easy and keeps the main function simple, we've decided to avoid a refactor.

This will allow use to remove the gioui.org dependency from the gogio command tests. The dependency is only required because of the test implementation from that commit, it's not required by gogio at all.

I'm adding some benchmark results here, so that I have some values to compare with the results that I get after the refactoring:

> perflock -governor 90% benchcmd EndToEnd/JS ./gogio.test -test.run=EndToEnd/JS
BenchmarkEndToEnd/JS    1       1646440344 ns/op        617933000 user-ns/op    135779000 sys-ns/op     111808512 peak-RSS-bytes
BenchmarkEndToEnd/JS    1       1666625151 ns/op        610389000 user-ns/op    143219000 sys-ns/op     111968256 peak-RSS-bytes
BenchmarkEndToEnd/JS    1       1649892298 ns/op        619959000 user-ns/op    140695000 sys-ns/op     112128000 peak-RSS-bytes
BenchmarkEndToEnd/JS    1       1650230940 ns/op        574351000 user-ns/op    183671000 sys-ns/op     111894528 peak-RSS-bytes
BenchmarkEndToEnd/JS    1       1631252744 ns/op        558702000 user-ns/op    193893000 sys-ns/op     112037888 peak-RSS-bytes


> perflock -governor 90% benchcmd EndToEnd/JS ./gogio.test -test.run=EndToEnd/JS
BenchmarkEndToEnd/JS    1       1654202639 ns/op        615574000 user-ns/op    155924000 sys-ns/op     111624192 peak-RSS-bytes
BenchmarkEndToEnd/JS    1       1640356104 ns/op        590132000 user-ns/op    168773000 sys-ns/op     111493120 peak-RSS-bytes
BenchmarkEndToEnd/JS    1       1618208652 ns/op        637713000 user-ns/op    133916000 sys-ns/op     112377856 peak-RSS-bytes
BenchmarkEndToEnd/JS    1       1648688801 ns/op        588096000 user-ns/op    173820000 sys-ns/op     112218112 peak-RSS-bytes
BenchmarkEndToEnd/JS    1       1670452693 ns/op        629662000 user-ns/op    138886000 sys-ns/op     112545792 peak-RSS-bytes


> perflock -governor 90% benchcmd EndToEnd/JS ./gogio.test -test.run=EndToEnd/JS
BenchmarkEndToEnd/JS    1       1653404277 ns/op        620253000 user-ns/op    144668000 sys-ns/op     111652864 peak-RSS-bytes
BenchmarkEndToEnd/JS    1       1648087313 ns/op        591378000 user-ns/op    163324000 sys-ns/op     111628288 peak-RSS-bytes
BenchmarkEndToEnd/JS    1       1649187530 ns/op        583183000 user-ns/op    169075000 sys-ns/op     111087616 peak-RSS-bytes
BenchmarkEndToEnd/JS    1       1633838453 ns/op        580090000 user-ns/op    178996000 sys-ns/op     111861760 peak-RSS-bytes
BenchmarkEndToEnd/JS    1       1681479761 ns/op        589006000 user-ns/op    177368000 sys-ns/op     111624192 peak-RSS-bytes
Status
REPORTED
Submitter
~schnwalter
Assigned to
No-one
Submitted
2 months ago
Updated
2 months ago
Labels
No labels applied.

~eliasnaur 2 months ago

On Fri Nov 13, 2020 at 7:16 AM CET, ~schnwalter wrote:

Global flags need to be avoided in order to add some tests, I will look into this sometime in the upcoming weeks.

After that, we can revisit this commit:

9bbeb92b cmd/gogio: reuse the test binary to call gogio in the e2e tests

It was created by ~mvdan, where he wrote:

An alternative here would have been to refactor main.go to allow being called directly. However, that would have required a non-trivial refactor, since flag parsing is done via globals. Given that the TestMain method is easy and keeps the main function simple, we've decided to avoid a refactor.

This will allow use to remove the gioui.org dependency from the gogio command tests. The dependency is only required because of the test implementation from that commit, it's not required by gogio at all.

I believe the usual way to refactor CLI tools such as gogio is to have a meaty (internal) sub-package and a small main package with global flags. Is that what you're planning to do?

Elias

~mvdan 2 months ago

I agree that the current method isn't perfect, but at the same time, the tests are end-to-end. Running a binary to perform the build should not be a problem, nor should it noticeably affect the cost of the tests.

If you do this for the sake of simplifying dependencies and the tests, I think it might be worth it. If you do this to speed up the tests, I don't think you'll get compelling results. Many of the tests run expensive programs that make an exec call comparatively trivial.

~schnwalter 2 months ago

I've looked over several cli tools (including go/src/cmd), for now, I've only added an initFlags() function that returns a struct which is passed to newBuildInfo(). The buildInfo contains more information and is now a global variable; this way, I avoided a lot of changes and we can still add more tests.

Register here or Log in to comment, or comment via email.