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

Enable AppVeyor CI for running test suite on Windows env #3315

Merged
merged 21 commits into from Aug 15, 2018
Merged

Conversation

dangra
Copy link
Member

@dangra dangra commented Jul 3, 2018

Test windows builds on Appveyor CI.
Closes #1684

@codecov
Copy link

codecov bot commented Jul 3, 2018

Codecov Report

Merging #3315 into master will decrease coverage by 0.03%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #3315      +/-   ##
==========================================
- Coverage   84.39%   84.36%   -0.04%     
==========================================
  Files         167      167              
  Lines        9361     9361              
  Branches     1390     1390              
==========================================
- Hits         7900     7897       -3     
- Misses       1206     1209       +3     
  Partials      255      255
Impacted Files Coverage Δ
scrapy/commands/startproject.py 100% <100%> (ø) ⬆️
scrapy/utils/python.py 82.68% <0%> (-1.68%) ⬇️

@kmike
Copy link
Member

kmike commented Jul 3, 2018

A related pull request: #1756, though I hope many of the workarounds from that PR are not needed now.

A ticket to close once we make it working: #1684.

@kmike kmike mentioned this pull request Jul 3, 2018
@kmike kmike added this to the v1.6 milestone Jul 4, 2018
@dangra dangra requested review from kmike and lopuhin August 15, 2018 12:24
@dangra
Copy link
Member Author

dangra commented Aug 15, 2018

all green @kmike ! ready to review and get it merged for 1.6 release 👯

shutil.rmtree(tmpdir)
# FIXME: Windows fails to remove the file because FeedExporter
# keeps a reference to the temporal file even after
# the spider finished.
Copy link
Member

Choose a reason for hiding this comment

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

Should we open an issue for that?

Copy link
Member Author

Choose a reason for hiding this comment

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

+1

Copy link
Member Author

Choose a reason for hiding this comment

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

# keeps a reference to the temporal file even after
# the spider finished.
try:
shutil.rmtree(tmpdir)
Copy link
Member

Choose a reason for hiding this comment

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

can we use rmtree(tmpdir, ignore_errors=True)?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good idea. fixing.

@kmike
Copy link
Member

kmike commented Aug 15, 2018

Awesome, thanks @dangra for picking this up and @jdemaeyer for the initial work!

@kmike kmike merged commit 91f986e into master Aug 15, 2018
@dangra
Copy link
Member Author

dangra commented Aug 15, 2018

TY @kmike.
Everyone knows @jdemaeyer is the master chef! I am here just to cleanup the dishes.

@dangra dangra deleted the test-on-windows branch August 15, 2018 16:20
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