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

Add Appveyor config for testing on Windows #81

Merged
merged 5 commits into from
May 7, 2017

Conversation

Mortal
Copy link
Contributor

@Mortal Mortal commented May 6, 2017

This PR adds an Appveyor config. When aio-libs/aiosmtpd is created on https://ci.appveyor.com (by the FLUFL), the config will be automatically picked up and tests will start running whenever master is updated.

Currently coverage fails because this if-branch is always taken in tests:

def eof_received(self):
    log.info('%r EOF received', self.session.peer)
    self._handler_coroutine.cancel()
    if self.transport is not None:                        # pragma: nopy34
        return super().eof_received()

I can't figure out how to make a test where eof_received() is called when self.transport is None, but it seems that this is exercised when testing on Linux by the tests in TestStartTLS (discovered by adding raise Exception() at the end of eof_received()).

@Mortal
Copy link
Contributor Author

Mortal commented May 6, 2017

You will want to go to https://ci.appveyor.com/notifications and disable email notifications after signing up for Appveyor.

@warsaw
Copy link
Contributor

warsaw commented May 7, 2017

I've enabled AppVeyor for aiosmtpd. I guess you're saying I want to disable email notifications because they'll be so spammy?

@warsaw
Copy link
Contributor

warsaw commented May 7, 2017

As for your first comment about eof_received(), yes it does happen in Linux, but I'm not sure if this isn't a bad bandaid. OTOH it looks like maybe it's not a problem for you any more? In any case, this is a great branch, thanks for fixing things on Windows.

(I wonder if we need to start moving all those *-coverage.ini files out of the top-level?)

@warsaw warsaw merged commit 4aaf2f4 into aio-libs:master May 7, 2017
@Mortal
Copy link
Contributor Author

Mortal commented May 7, 2017

Yeah, I personally think it's too much with an email per failed build.

The eof_received() coverage problem still persists. It seems like a bad band-aid to me -- I couldn't exactly figure out under which circumstances self.transport would become None, so I would say it needs an explanation in the code at the very least.

I think the *-coverage.ini should be moved out of the top-level. I tried to figure out if I could avoid having six distinct files, but I don't know enough about coverage to say if that is possible.

@warsaw
Copy link
Contributor

warsaw commented May 7, 2017 via email

@warsaw
Copy link
Contributor

warsaw commented May 7, 2017

My suspicion is that the traceback on eof_received() is a bug in asyncio. It actually doesn't affect the outcome of the aiosmtpd tests, but it's annoying console output. I'm going to file the bug in upstream Python and we'll see where that leads, but for now I can live with it.

Oh, and here's where I make a plea to upstream coverage to handle the multiple coverage.ini files better.

@Mortal Mortal deleted the appveyor branch May 7, 2017 23:17
@warsaw warsaw mentioned this pull request May 7, 2017
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.

2 participants