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 CI config #7

Closed
wants to merge 2 commits into from
Closed

add appveyor CI config #7

wants to merge 2 commits into from

Conversation

Borda
Copy link

@Borda Borda commented Oct 1, 2019

later also add the badge (using mine just for this branch Build status )
see sample https://github.com/ogrisel/python-appveyor-demo/blob/master/appveyor.yml

@codecov
Copy link

codecov bot commented Oct 1, 2019

Codecov Report

Merging #7 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@          Coverage Diff          @@
##           master     #7   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files           4      4           
  Lines         414    414           
  Branches       82     82           
=====================================
  Hits          414    414

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1dfa0d4...3436435. Read the comment docs.

@Borda
Copy link
Author

Borda commented Oct 1, 2019

python --version
Python 2.7.16
pip --version
pip 19.2.3 from c:\python27-x64\lib\site-packages\pip (python 2.7)

but later

You are using pip version 8.1.1, however version 19.2.3 is available.

See:

@avylove
Copy link
Contributor

avylove commented Oct 2, 2019

I'm glad you got your build working. And I do think Enlighten needs some testing on Windows, but I think you're missing the goal here. Adding an Appveyor config is pretty easy, the hard part is coming up with relevant tests to run on Windows. Most of the testing involves creating PTYs which is easy on Linux, but the capability was just added to Windows 10 a year ago (It's also in Server 2019). So the options are to make the current tests work by figuring out how to use a similar capability and run the tests on Server 2019 or create a general use test essentially to make sure nothing breaks. Probably both would be good to have in the longer term, though a general use test would be easier to implement in the short term. Perhaps running one of the example scripts would be sufficient.

If you're going to work on this, that would be awesome, otherwise I'll add it to my todo list and close this PR.

@Borda
Copy link
Author

Borda commented Oct 2, 2019

Well, I have started with adding working config and it seems that the tests are not passing so I was wondering if you are going to fix them before we would add another test...

ERROR: test_counter (unittest.loader._FailedTest)
----------------------------------------------------------------------
ImportError: Failed to import test module: test_counter
Traceback (most recent call last):
  File "c:\python35-x64\Lib\unittest\loader.py", line 428, in _find_test_path
    module = self._get_module_from_name(name)
  File "c:\python35-x64\Lib\unittest\loader.py", line 369, in _get_module_from_name
    __import__(name)
  File "C:\projects\enlighten\tests\test_counter.py", line 19, in <module>
    from tests import TestCase, mock, MockManager, MockTTY, MockCounter, MockBaseCounter
  File "C:\projects\enlighten\tests\__init__.py", line 13, in <module>
    import fcntl
ImportError: No module named 'fcntl'

@avylove
Copy link
Contributor

avylove commented Oct 2, 2019

As explained before, the tests aren't broken, they use lower level Linux calls to create a pseudo terminal. They weren't designed to run on Windows and making them run on Windows is not a minor effort. A basic functionality test would be the place to start rather than trying to run the whole test suite. So if you'd like to tackle that, it would be appreciated, but otherwise I think this request is putting the cart before the horse.

@avylove avylove closed this Oct 26, 2019
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