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

3757 : Refactored test_introducer in web tests #1100

Merged
merged 5 commits into from Aug 11, 2021

Conversation

Fenn-CS
Copy link
Collaborator

@Fenn-CS Fenn-CS commented Aug 8, 2021

Refactored test_introducer.py in test/web module to either user SyncTestCase or AsyncTestCase.

Ticket : https://tahoe-lafs.org/trac/tahoe-lafs/ticket/3757

…se test cases

Signed-off-by: fenn-cs <fenn25.fn@gmail.com>
Copy link
Member

@exarkun exarkun left a comment

Choose a reason for hiding this comment

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

Thanks. I left some commentary inline. This looks like a good start.

src/allmydata/test/web/test_introducer.py Outdated Show resolved Hide resolved
src/allmydata/test/web/test_introducer.py Outdated Show resolved Hide resolved
src/allmydata/test/web/test_introducer.py Outdated Show resolved Hide resolved
src/allmydata/test/web/test_introducer.py Outdated Show resolved Hide resolved
src/allmydata/test/web/test_introducer.py Outdated Show resolved Hide resolved
Signed-off-by: fenn-cs <fenn25.fn@gmail.com>
@Fenn-CS Fenn-CS force-pushed the 3757.refactor.web-test-introducer branch from b169868 to 7ad3fa9 Compare August 9, 2021 22:46
Copy link
Contributor

@meejah meejah left a comment

Choose a reason for hiding this comment

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

Looking really good!

I'm not asking for changes here, but especially for shorter tests like this one I think we (or at least "I") have been indenting things like this:

self.assertThat(
    render(resource, {b"t": [b"json"]}),
    succeeded(
        AfterPreprocessing(
            json.loads,
            Equals({
                u"subscription_summary": {"arbitrary": 2},
                u"announcement_summary": {"arbitrary": 1},
            })
        )
    )
)

I'm actually curious if you think that counts as "readable", or if it's just because I'm now used to it ;)

If nothing else, I do like:

self.assertThat(
    <something>,
    CollectionOfMatchers(),
)

...to separate the "test thing" (<something>) from the "expected results part" (i.e. all the testtools matchers). I do think it took me a while to get around the "matcher" style of test-tools, but it can be fairly readable (even if the writing can be a little harder, due to having to look up matchers).

p.s. I know I said "approve" but maybe @exarkun wishes to followup .. and CI has to be green before we merge things...

@Fenn-CS
Copy link
Collaborator Author

Fenn-CS commented Aug 10, 2021

@meejah Yeah, I think that's more readable. I ran the linting I found the tox config to see if would complain or refactor that, did not! Could quickly push a new commit with that. If we would use it in other places we might as well just do it here.

EDIT : It's worth noting that I think the use of expected and response make it the code more readable, but also spreading it over a few lines is a good idea. So a kind of a mix.

Signed-off-by: fenn-cs <fenn25.fn@gmail.com>
@coveralls
Copy link
Collaborator

coveralls commented Aug 10, 2021

Coverage Status

Coverage increased (+0.02%) to 95.474% when pulling b5c3290 on Fenn-CS:3757.refactor.web-test-introducer into 22622b5 on tahoe-lafs:master.

@Fenn-CS Fenn-CS closed this Aug 10, 2021
@Fenn-CS Fenn-CS reopened this Aug 10, 2021
@exarkun exarkun closed this Aug 10, 2021
@exarkun exarkun reopened this Aug 10, 2021
Signed-off-by: fenn-cs <fenn25.fn@gmail.com>
@Fenn-CS Fenn-CS closed this Aug 11, 2021
@Fenn-CS Fenn-CS reopened this Aug 11, 2021
@May-Lee
Copy link
Contributor

May-Lee commented Aug 11, 2021

@exarkun could you approve requested changes and merge when ready please?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants