On Sat, Jun 20, 2015 at 11:50 PM, Mads Kiilerich <[email protected]> wrote: > On 06/20/2015 10:37 PM, Thomas De Schampheleire wrote: >> >> Some trivial cleanups related to hardcoded strings in tests. > > > I think another school of thought is that tests should be as simple, static > and explicit as possible. To avoid begging the question they are asking, > they should avoid any computation in the tests. Having hardcoded 'expected' > strings will also make it easier when debugging.
In principle I agree: tests should be completely stand-alone. Changing a hardcoded string in one test should not impact any other test. However, the test users and test repositories are set up at the very beginning of all tests (see kallithea/lib/db_manage.py). They are shared state. When a test uses the username 'test_admin', it cannot freely change that without impacting the test. The test depends on a very specific string from that pre-created database. This dependency is, however, implicit, and now made explicit with these patches. In this light, I think the patches are an improvement to the current situation. The end goal, I think, should be to get rid of this prepopulated database, and instead use pytest fixtures explicitly for each test. The fixture could start from a created, but otherwise completely empty database. When a test requires a user to be created, it would explicitly ask for it using a second fixture (or with a helper method). When this refactoring is made, constants such as TEST_USER_ADMIN_LOGIN can and should be removed, and each test can work with its own hardcoded strings. I'm not sure about the performance impact of this approach: does the database need to be recreated each test or can it be rolled back before starting a new test so that each test starts with the same initial state? I'm adding Brianna and Marc in this thread, they probably have some wise input on this topic :) /Thomas _______________________________________________ kallithea-general mailing list [email protected] http://lists.sfconservancy.org/mailman/listinfo/kallithea-general
