Skip to content

Conversation

ear7h
Copy link
Contributor

@ear7h ear7h commented Sep 17, 2019

The changes in this pull request are actually part of #69 but I don't see that PR being merged soon (it's complicated and there's bigger fish to fry) but, I'd really like this patch so that tests can be compiled/run in a reasonable amount of time.

The only changes in this PR are build tags to the files which contain or organize tiles in the testing directory.

@ear7h ear7h requested review from ARolek and gdey September 17, 2019 06:39
@coveralls
Copy link

coveralls commented Sep 17, 2019

Pull Request Test Coverage Report for Build 313

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 58.009%

Totals Coverage Status
Change from base Build 287: 0.0%
Covered Lines: 17286
Relevant Lines: 29799

💛 - Coveralls

@gdey
Copy link
Member

gdey commented Sep 17, 2019

I have not reviewed this, but just from the description, this is the wrong direction. There are already command lines params for running quick tests and the testing support for this. We should be using t.Skip and t.Short() for this type of things. Then on the command line do: go test -short for quicker tests.

@ARolek
Copy link
Member

ARolek commented Sep 18, 2019

@gdey this is a really small PR, can you give it another look? I'm not quite sure if t.Skip() or t.Short() would be appropriate here. I think the main issue that @ear7h is trying to address is skipping the compiler from including these geometries unless the developer is explicitly interested in working on them.

@gdey
Copy link
Member

gdey commented Sep 18, 2019

What are you trying to do with this PR? What is the problem you are trying to address? Adding the build tag will force any tests that is making use of these cases to include that tag as well, otherwise, things will not compile.

@ear7h
Copy link
Contributor Author

ear7h commented Sep 19, 2019

@gdey The goal is what @ARolek mentioned. The files that I added the build tags to are >100MB together and compiling them takes an unnecessarily long time since the geometries end up not being used in tests. This PR is especially helpful for CI because it runs go test .;/... 6 times with no build cache each time.

Here's the output from time for the two branches:

master $ CGO_ENABLED=0 time go test ./...
...
       71.32 real       120.84 user        12.42 sys

test-tile-tag $ CGO_ENABLED=0 time go test ./...
...
        9.33 real        25.04 user         5.11 sys

@ear7h ear7h mentioned this pull request Sep 24, 2019
3 tasks
@ear7h
Copy link
Contributor Author

ear7h commented Sep 25, 2019

After some discussion, we found the best solution would be to decode the geometry at runtime, storing it as a string in source code. This would prevent odd bugs from using build tags while keeping the compilation speed of tests relatively quick.

I've implemented a patch on top of what's sitting in the wkt: improvements PR. And it's slightly more complicated than expected, because we cannot have the circular dependency wkt -> testing ->wkt. But, I don't think this is a problem because it communicates the need for a compilation step rather than mindlessly accessing gtesting.Tiles() (this might lead to bugs when running benchmarks).

@ear7h ear7h changed the base branch from master to simplify-improvements September 25, 2019 01:35
@ear7h ear7h changed the base branch from simplify-improvements to master September 26, 2019 01:00
@gdey
Copy link
Member

gdey commented Sep 27, 2019

@ear7h did you get all the package tests passing?

* the tile geometries are now stored as a wkt string
* running tests/benchmarks with the geometries is done by compiling them
first
* fixed bug in wkt decoder which did not process collections properly
Copy link
Member

@gdey gdey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@gdey gdey merged commit 91d4041 into master Sep 30, 2019
@gdey gdey deleted the test-tile-tag branch September 30, 2019 19:23
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.

4 participants