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

[travis] Build coverage after success, separately #333

Closed
wants to merge 5 commits into from

Conversation

@Komzpa
Copy link
Member

commented Nov 13, 2018

No description provided.

@strk strk closed this in 6d3201c Nov 13, 2018

@dbaston

This comment has been minimized.

Copy link
Member

commented Nov 13, 2018

@Komzpa What is the purpose of making a pull request if you're going to merge it before anyone has a chance to look at it? I thought the idea of this seemed good, but then it evolved beyond the title of the PR to also reduce the scope of the testing with no benefit on build time. Your build time is only as fast as the longest entry in your matrix, so removing debug-mode testing (just hours after I added it, and with no discussion) and removing coverage from all but one of the builds doesn't make anything faster.

@Komzpa

This comment has been minimized.

Copy link
Member Author

commented Nov 13, 2018

@dbaston have a second look - debug-testing is there, as a separate build. Now instead of building PostGIS, testing, and rebuilding with debug it is only building and testing, and in parallel building as debug, finishing the debug build in just under 3 minutes.

Coverage build is taking the same time as normal build, it's not building anything but coverage. In the scheme you propose there's a seven-minute window when build may be marked green but coverage is not ready yet. In what's been committed it ends roughly in the same time as the other builds.

@Komzpa Komzpa deleted the Komzpa:travis_codecov_after_success branch Nov 13, 2018

@dbaston

This comment has been minimized.

Copy link
Member

commented Nov 13, 2018

@Komzpa sorry, I don't see it. I only see debug and coverage testing against Postgres 11/GEOS 3.7, not all of the configurations. Am I missing something?

@Komzpa

This comment has been minimized.

Copy link
Member Author

commented Nov 13, 2018

@dbaston in your previous message you said "removing debug-mode testing (just hours after I added it, and with no discussion)". It's there, and now you see it.

Indeed it is just enabled for one configuration - what is the expected catch for doing this in all of them versus doing just for one of them, the latest one?

What is the benefit of coverage report for dusty versions of libraries? A bug debugged by coverage isn't going to be in libraries anyway.

I'd say we need to free up some space in our buildtimes to squeeze undefined behaviour sanitizer and probably address sanitizer in.

@dbaston

This comment has been minimized.

Copy link
Member

commented Nov 13, 2018

@Komzpa the point of testing each configuration is because the code is full of preprocessor directives that execute different code paths depending on the version of various dependencies, and you want to make sure that the code compiles (in standard and debug modes) under all of those options.

I'm extremely bothered by this way of working, where you clobber work committed by other developers without allowing a chance for review and without explaining, documenting, or justifying the changes that you're making. Is this how you're accustomed to working on other multi-developer projects?

@Komzpa

This comment has been minimized.

Copy link
Member Author

commented Nov 14, 2018

@dbaston with all respect, I believe you are overreacting and following "you did it not the way I imagined so I'll revert it" flawed logic.

Functionality is there and wasn't removed; adding it to the build matrix to have all the configurations checked is a matter of a couple of lines in travis env:. You can add them instead of pushing people's faces into dirt, can't you?

@dbaston

This comment has been minimized.

Copy link
Member

commented Nov 14, 2018

@Komzpa You're asking me to spend additional time on this, changing the code to get it back to where I want it in a way that still preserves the valuable parts of what you added. But the burden is on you to address problems with pull requests before they're merged, not on other developers to fix them afterwards.

I'm not sure where "pushing people's faces into dirt" comes from. All I am asking is for you to work in a way that is respectful of other developers, by providing an opportunity for review and comment before pull requests are merged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.