Skip to content

Pet opt-in#402

Closed
ForNeVeR wants to merge 7 commits into
developfrom
feature/pet-opt-in
Closed

Pet opt-in#402
ForNeVeR wants to merge 7 commits into
developfrom
feature/pet-opt-in

Conversation

@ForNeVeR

@ForNeVeR ForNeVeR commented Jul 1, 2016

Copy link
Copy Markdown
Member

After this change, horta administrator will have to explicitly enable pet for the rooms where he wants to use it.

That's actually a breaking change, but we're still in major version 0, so we're allowed to make such changes without incrementing the major version.

Closes #400.

@ForNeVeR ForNeVeR assigned rexim and ForNeVeR and unassigned rexim Jul 1, 2016
@ForNeVeR

ForNeVeR commented Jul 1, 2016

Copy link
Copy Markdown
Member Author

I need to investigate the failing tests. According to my current experiments, they are failing seemingly at random. Will report back after the investigation.

@hagane

hagane commented Jul 1, 2016

Copy link
Copy Markdown
Contributor

I know that failing test. It's failing because it has a seemingly arbitrary timeout. You can safely ignore it or, if you're serious about it, set it to not run in parallel with other test suites.

@hagane

hagane commented Jul 2, 2016

Copy link
Copy Markdown
Contributor

The test you added -- the one in PetPluginSpec -- looks genuinely broken, though.

@ForNeVeR

ForNeVeR commented Jul 2, 2016

Copy link
Copy Markdown
Member Author

Got it. Our static configuration is not the best candidate to be mocked during the unit tests. The problem is it gets configured only once per all the tests, and the resulting configuration is baked into static Configuration.properties object first time it's accessed.

I'll add some logic to globally deinitialize it for now, but we'll need to invent something more testable in the future (so we could replace or mock that configuration object on a per-plugin basis). Will file an issue for that.

@ForNeVeR

ForNeVeR commented Jul 2, 2016

Copy link
Copy Markdown
Member Author

Ah, I've just added these lines to the global config. It was the easiest way.

@ForNeVeR ForNeVeR assigned rexim and unassigned ForNeVeR Jul 2, 2016
@rexim

rexim commented Jul 2, 2016

Copy link
Copy Markdown
Member

This is pretty good shit! I like it! Approved.

@rexim rexim assigned ForNeVeR and unassigned rexim Jul 2, 2016
@ForNeVeR

ForNeVeR commented Jul 3, 2016

Copy link
Copy Markdown
Member Author

Thanks all. Will merge and release the new version soon.

@ForNeVeR ForNeVeR closed this Jul 3, 2016
@ForNeVeR
ForNeVeR deleted the feature/pet-opt-in branch July 3, 2016 04:30
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.

3 participants