address D94 comments
ClosedPublic

Authored by kparal on May 13 2014, 2:02 PM.

Details

Summary

The config now has temporary directories set during test suite
initialization. A change from instance attributes to class attributes
was required in Config in order to achieve this easily.

This should solve T163. This can replace D94.

Test Plan

Tests pass

Diff Detail

Lint
Lint Skipped
Unit
Unit Tests Skipped

In case you wondered - this cannot be done as a global fixture. Even session-scoped fixtures are executed after methods like setup_method(). That's very unfortunate, but that's the current implementation of pytest. Therefore we can't even use tmpdir, monkeypatch and similar fixtures (not to mention that they can be called only from function-scoped fixtures).

Still not thrilled that we have to do this but it certainly removes the need for some ugly test code that would be present with some other solutions. Looks good overall, just some questions.

testing/test_bodhi_comment_directive.py
16 ↗(On Diff #264)

why do the setup method this way instead of the builtin setup_method?

testing/test_resultsdb_directive.py
59 ↗(On Diff #264)

Why is does the config need to be reset for every run here when it wasn't needed before?

kparal added inline comments.May 13 2014, 3:52 PM
testing/test_bodhi_comment_directive.py
16 ↗(On Diff #264)

The Config is modified here (Config.reporting_enabled = True etc), and there's no cleanup code. The following modules receive an altered object, which can cause some hard-to-debug side effects. Monkeypatch takes care of that and resets the object after these tests finish.

testing/test_resultsdb_directive.py
59 ↗(On Diff #264)

Ditto as above, to avoid side effects in following modules. Each module should clean after itself. Or we need to take other measures (e.g. have a global fixture that resets config._config between different modules).

tflink accepted this revision.May 13 2014, 4:05 PM

I still don't like the fact that we have to do this (fs access for unit tests, not the code here) but as I have no better alternative suggestions, I can't complain :)

testing/test_bodhi_comment_directive.py
16 ↗(On Diff #264)

After talking over IRC - the reason for using the explicit fixtures instead of the built-in xunit style setup_method is that those built-in mehtods can't use monkeypatch or tmpdir

kparal closed this revision.May 14 2014, 1:06 PM

Closed by commit rLTRN49c270fd23a7 (authored by @kparal).

Fixed in three commits, Phab shows just the oldest one.