Opened 5 years ago

Closed 5 years ago

#6205 closed defect (fixed)

integration tests fail on Windows

Reported by: reganeet Owned by: reganeet
Priority: Medium Milestone:
Component: Core Tor/Stem Version:
Severity: Keywords:
Cc: Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

Some integration tests on Windows fail, and here is a fix:
https://github.com/beckbaibai/stem/tree/testwin

Child Tickets

Change History (10)

comment:1 Changed 5 years ago by Sebastian

  • Status changed from new to needs_review

comment:2 Changed 5 years ago by atagar

  • Status changed from needs_review to needs_information

Looks great! Only a couple questions...

  1. stem/util/system.py expand_path - Don't we need the drive letters for a path to be absolute? That was the bit that I suspected would be difficult, though if it's not necessary then great.
  1. test/integ/util/system.py - Why are you skipping these tests if the pgrep lookup fails? It's just checking if there are multiple tor instances, which we could probably default to 'False'.

Cheers! -Damian

comment:3 Changed 5 years ago by reganeet

Don't we need the drive letters for a path to be absolute? 

Yes we do; os.getcwd() will provide an absolute path with the drive letter. Are you suggesting that when a path is provided, we should append it to the current drive letter and then check if it is absolute?

Why are you skipping these tests if the pgrep lookup fails?

Huh, I thought it is required for the test cases to run. I was wrong. Can we set the default value of "is_extra_tor_running" to False, get rid of "if self.is_extra_tor_running is None" in setUp(), and simply pass it on windows?

Beck

comment:4 Changed 5 years ago by atagar

  • Status changed from needs_information to needs_revision

Yes we do; os.getcwd() will provide an absolute path with the drive letter.

Ahh, gotcha.

Can we set the default value of "is_extra_tor_running" to False, get rid of "if self.is_extra_tor_running is None" in setUp()

Simplest is probably just to have an 'is_windows()' check and hardcode it to False in that case. It would be nice if we could later check this on Windows, but not at all vital.

Pushed your changes with some minor tweaks...
https://gitweb.torproject.org/stem.git/commitdiff/6f626f3

Would you mind exercising these new Windows expand_path() use cases in the unit tests? If you look in 'test/unit/util/system.py' you'll find a 'test_expand_path_unix' test. It would be great to have a windows counterpart for this. :)

comment:5 Changed 5 years ago by reganeet

Pushed your changes with some minor tweaks...

Some test cases in test/integ/util/system.py now fail. Do you want me to fix them?

It would be great to have a windows counterpart for this.

No problem.

comment:6 Changed 5 years ago by atagar

Some test cases in test/integ/util/system.py now fail. Do you want me to fix them?

Yes please. :)

comment:7 Changed 5 years ago by reganeet

Hi atagar:

In stem/util/system.py line 512, why did you add an is_windows() check? This makes the method fail to expand tildas on windows. os.path.expanduser(relative_path) works perfectly on windows:

>>> os.path.expanduser("~")
'C:\\Users\\Beck'

So imo, we should remove this is_windows() check.

Beck

comment:8 Changed 5 years ago by atagar

Oh, didn't realize that worked on windows. Feel free to put it back.

comment:9 Changed 5 years ago by reganeet

Fixed test cases on windows and added test_expand_path_windows:
https://github.com/beckbaibai/stem/tree/testwin

comment:10 Changed 5 years ago by atagar

  • Resolution set to fixed
  • Status changed from needs_revision to closed

Fixes pushed. Thanks!

Note: See TracTickets for help on using tickets.