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 duplicate building tags #248

Merged
merged 2 commits into from
Oct 19, 2018
Merged

Remove duplicate building tags #248

merged 2 commits into from
Oct 19, 2018

Conversation

isabelleh
Copy link
Contributor

Description:

This is a quick fix to my previous PR which introduced a bug by duplicating a number of tags in the VALID_BUILDINGS enum set. #244

Potential Impact:

This will effect code calling the isBuilding method.

Unit Test Approach:

No additional unit testing.

Test Results:

Passes integration tests.


In doubt: Contributing Guidelines

@MikeGost MikeGost added the Bug label Oct 19, 2018
Copy link
Contributor

@MikeGost MikeGost left a comment

Choose a reason for hiding this comment

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

Thanks for the fix, @isabelleh!

@MikeGost
Copy link
Contributor

Oh no, testParallelPerformance failed @adahn6. Might be worth looking into it.

@adahn6
Copy link
Contributor

adahn6 commented Oct 19, 2018

@MikeGost oh no >.< alright, I think it's probably best to just comment out that test then? It's still somewhat useful as a performance check, but clearly not reliable enough to be included by default

@MikeGost
Copy link
Contributor

@adahn6 - Let’s add the @ignore annotation for now and create an issue to replace it with something better or remove altogether.

@lucaspcram
Copy link
Contributor

I too have been bitten by my own questionable multithreaded tests :)

jwpgage
jwpgage previously approved these changes Oct 19, 2018
Copy link
Contributor

@jwpgage jwpgage left a comment

Choose a reason for hiding this comment

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

Thanks for the fix, @isabelleh!

@MikeGost
Copy link
Contributor

@isabelleh - Do you mind adding the @Ignore tag to the failing test in this PR - that way we can merge it and remove the chance of a failing build in Travis. Meanwhile, I'll create an issue to address this.

@MikeGost
Copy link
Contributor

Relevant issue: #250

Copy link
Contributor

@MikeGost MikeGost left a comment

Choose a reason for hiding this comment

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

Requesting changes to add Ignore tag.

Copy link
Contributor

@MikeGost MikeGost left a comment

Choose a reason for hiding this comment

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

Thank you!

@MikeGost MikeGost merged commit e6dba01 into osmlab:dev Oct 19, 2018
@matthieun matthieun added this to the 5.2.0 milestone Oct 24, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants