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

Fix test suite #19

Closed
wants to merge 6 commits into from
Closed

Fix test suite #19

wants to merge 6 commits into from

Conversation

vlcinsky
Copy link

@vlcinsky vlcinsky commented May 8, 2018

Trying to run test suite, some cases were failing.

PR is fixing these cases:

  • detection of hg/git caused failure if some was missing
  • trailing space in logs were removed by editor and caused failing case. Test modified to make the significant trailing space clearly visible and prone to removal by an editor.

As some editors remove trailing spaces, some tests were failing after
test case code was saved. Now the significant space is explicitly
rendered using `"text%(space)s" % dict(space=" ")`
@ekohl
Copy link
Collaborator

ekohl commented Jun 2, 2018

Apologies for not responding earlier. At first glance this looks good and a neat cleanup but Appveyor is still unhappy. Have you had a look at it?

@vlcinsky
Copy link
Author

vlcinsky commented Jun 2, 2018

Reading logs form appveyor it looks, like following problems are present:

  • console encoding: if console does not use utf-8, it might have problems to encode output of some chars to console and fails with that nasty encode problem error.
  • problem to open a file: Windows typically locks the file used and other processes might have problems to access it. It reminds me the situation, when my "windows based" college cannot delete a file because it is already open in editor.
  • attempt to write a stream to closed file: no idea what is going on.

As I do not have access to MS Windows, I am unable to investigate this easily.

Can some MS Windows user investigate the problems? I guess running $ tox would serve the purpose (of course with proper investigation of what is going on).

@ekohl
Copy link
Collaborator

ekohl commented Jun 2, 2018

There is a universal_newlines=True in `subprocess that I think handles the encoding as well. Perhaps that would clean up the code enough?

@ekohl
Copy link
Collaborator

ekohl commented Aug 1, 2018

I forgot this was still open and created #27 as well. Given the Windows issues, should we even try to support it?

@jeremycarroll
Copy link
Contributor

I fixed the appveyor tests. They all seemed genuine bugs, and the fixes were in the code not the tests

@ekohl
Copy link
Collaborator

ekohl commented Aug 29, 2018

Closing in favor of the other PR

@ekohl ekohl closed this Aug 29, 2018
@vlcinsky vlcinsky deleted the test_fix branch August 30, 2018 09:00
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.

None yet

3 participants