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

tests: Run functional test on Windows and enable it on Appveyor #14007

Merged
merged 2 commits into from Sep 24, 2018

Conversation

Projects
None yet
8 participants
@ken2812221
Copy link
Member

commented Aug 20, 2018

This PR do the following things:

  • Make functional tests compatible with Windows
  • Print color output in functional tests for Windows 10
  • Run util and functional tests on appveyor
  • Do not run symlink tests on Windows

Note:

  • The wallet_multiwallet.py fail is unrelated to the test framework, it's a bug related to c++ code or maybe dependencies. bitcoind would exit with 0xC0000005(Access violation) during shutdown occasionally. Disable this for now.
  • Not using --failfast because this is still in experimental. We should track if there is any other error.
  • Disable ZMQ tests because the python zmq library could cause access violation sometimes.
  • Disable feature_notifications because Bitcoin Core handles the command in different thread, whicha can cause a race condition.

@fanquake fanquake added the Tests label Aug 20, 2018

@ken2812221 ken2812221 force-pushed the ken2812221:patch-2 branch 9 times, most recently Aug 20, 2018

@MarcoFalke

This comment has been minimized.

Copy link
Member

commented Aug 22, 2018

I think there is some interference with a firewall or something that kills the bitcoind processes?

appveyor.yml Outdated

python -m pip install zmq 2>&1 | %{ "$_" }

python test\\functional\\test_runner.py --force --combinedlogslen=4000 --quiet

This comment has been minimized.

Copy link
@MarcoFalke

MarcoFalke Aug 22, 2018

Member

Could as a first step only run the util tests?

This comment has been minimized.

Copy link
@ken2812221

ken2812221 Aug 23, 2018

Author Member

You mean to drop away functional tests and replace it with util tests from this PR, and re-add the funfctional tests in another PR?

@ken2812221 ken2812221 force-pushed the ken2812221:patch-2 branch 5 times, most recently Aug 23, 2018

@ken2812221

This comment has been minimized.

Copy link
Member Author

commented Aug 23, 2018

It is really hard to see blue color on powershell, so change the color to green
image

@DrahtBot

This comment has been minimized.

Copy link
Contributor

commented Aug 23, 2018

Note to reviewers: This pull request conflicts with the following ones:
  • #14275 (tests: Write the notification message to different files to avoid race condition in feature_notifications.py by ken2812221)
  • #14241 (appveyor: trivial build cache improvement by ken2812221)
  • #14237 (Fix for test runner UnicodeDecodeError when utf-8 is not supported by sanket1729)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

@DrahtBot DrahtBot referenced this pull request Aug 23, 2018

Closed

build: make 'depends' output less spammy #13654

1 of 1 task complete
@ken2812221

This comment has been minimized.

Copy link
Member Author

commented Aug 23, 2018

I think there is some interference with a firewall or something that kills the bitcoind processes?

I agree with you, they are all passed on my Windows PC.

@MarcoFalke

This comment has been minimized.

Copy link
Member

commented Aug 23, 2018

They didn't pass for me on Windows 7

@ken2812221 ken2812221 force-pushed the ken2812221:patch-2 branch Aug 23, 2018

@ken2812221

This comment has been minimized.

Copy link
Member Author

commented Aug 24, 2018

Oh, it's occasionally. I tested to run it 10 times, 8 times failed, 2 times succeed.

@ken2812221 ken2812221 force-pushed the ken2812221:patch-2 branch 6 times, most recently Aug 24, 2018

test/functional/test_framework/authproxy.py Outdated
@@ -102,6 +96,8 @@ def _request(self, method, path, postdata):
'User-Agent': USER_AGENT,
'Authorization': self.__auth_header,
'Content-type': 'application/json'}
if os.name == 'nt': # Windows somehow does not like to re-use connections

This comment has been minimized.

Copy link
@laanwj

laanwj Sep 13, 2018

Member

I guess this is meant as a temporary work-around until we find out why? (might want to add a comment in that regard) being able to re-use RPC connections is a big deal, it's not something we should simply accept as-is.

@ken2812221 ken2812221 force-pushed the ken2812221:patch-2 branch 2 times, most recently to d567847 Sep 13, 2018

@laanwj

This comment has been minimized.

Copy link
Member

commented Sep 13, 2018

utACK d567847

@ken2812221 ken2812221 force-pushed the ken2812221:patch-2 branch 2 times, most recently Sep 16, 2018

@ken2812221

This comment has been minimized.

Copy link
Member Author

commented Sep 16, 2018

Update: Disabled 1 additional tests for now because of #14225 merged.

@ken2812221 ken2812221 force-pushed the ken2812221:patch-2 branch Sep 17, 2018

@ken2812221

This comment has been minimized.

Copy link
Member Author

commented Sep 17, 2018

@MarcoFalke connection is still used in mining_getblocktemplate_longpoll.py. Please re-review.

@ken2812221 ken2812221 force-pushed the ken2812221:patch-2 branch Sep 17, 2018

@ken2812221 ken2812221 force-pushed the ken2812221:patch-2 branch to 661ac15 Sep 17, 2018

@MarcoFalke

This comment has been minimized.

Copy link
Member

commented Sep 17, 2018

utACK 661ac15

@ken2812221 ken2812221 force-pushed the ken2812221:patch-2 branch 2 times, most recently to 661ac15 Sep 21, 2018

@MarcoFalke MarcoFalke merged commit 661ac15 into bitcoin:master Sep 24, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

MarcoFalke added a commit that referenced this pull request Sep 24, 2018

Merge #14007: tests: Run functional test on Windows and enable it on …
…Appveyor

661ac15 appveyor: Run functional tests on appveyor (Chun Kuan Lee)
2148c36 tests: Make it possible to run functional tests on Windows (Chun Kuan Lee)

Pull request description:

  This PR do the following things:
  - Make functional tests compatible with Windows
  - Print color output in functional tests for Windows 10
  - Run util and functional tests on appveyor
  - Do not run symlink tests on Windows

  Note:
  - The wallet_multiwallet.py fail is unrelated to the test framework, it's a bug related to c++ code or maybe dependencies. `bitcoind` would exit with 0xC0000005(Access violation) during shutdown occasionally. Disable this for now.
  - Not using `--failfast` because this is still in experimental. We should track if there is any other error.
  - Disable ZMQ tests because the python zmq library could cause access violation sometimes.
  - Disable `feature_notifications` because Bitcoin Core handles the command in different thread, whicha can cause a race condition.

Tree-SHA512: b76db137d264e62a5c130e1cbca7a2ca002a7a0f4153fa0b92c1ea6c9c09ef0533e11c49bdbd566c472d8ff59f245758feb5e5a6ec6cb6bb66a1c67bab5fa48a

@ken2812221 ken2812221 deleted the ken2812221:patch-2 branch Sep 24, 2018

@jnewbery
Copy link
Member

left a comment

tested ACK. I think there's one change that needs to be reverted. @ken2812221 - can you take a look for me?

@@ -264,7 +285,7 @@ def main():

# Remove the test cases that the user has explicitly asked to exclude.
if args.exclude:
exclude_tests = [re.sub("\.py$", "", test) + ".py" for test in args.exclude.split(',')]
exclude_tests = [re.sub("\.py$", "", test) + (".py" if ".py" not in test else "") for test in args.exclude.split(',')]

This comment has been minimized.

Copy link
@jnewbery

jnewbery Sep 24, 2018

Member

I don't understand this change. The point of:

[re.sub("\.py$", "", test) + ".py"

is that if <test_name>.py or <test_name> is given, then it's normalized to <test_name>.py.

Your new version:

re.sub("\.py$", "", test) + (".py" if ".py" not in test else "")

essentially toggles, so <test_name>.py is changed to <test_name> and <test_name> is changed to <test_name>.py.

>>> test1 = 'test_name1.py'
>>> test2 = 'test_name2'
>>> for test in [test1,test2]:
...     print(re.sub("\.py$", "", test) + ".py") # original version
... 
test_name1.py
test_name2.py
>>> for test in [test1,test2]:
...     print(re.sub("\.py$", "", test) + (".py" if ".py" not in test else "")) # new version
... 
test_name1
test_name2.py

This breaks using -exclude=<test_name>.py

This comment has been minimized.

Copy link
@MarcoFalke

MarcoFalke Sep 24, 2018

Member

The goal was to be able to specify --exclude "feature_notifications,wallet_multiwallet,wallet_multiwallet.py --usecli".

I think removing everything from the test_list that .startswith(item.split('.py')[0]) for any item in args.exclude.split(',') should solve all issues?

This comment has been minimized.

Copy link
@ken2812221

ken2812221 Sep 25, 2018

Author Member

Oh, I didn't test that case. Should be fixed in #14316

MarcoFalke added a commit to MarcoFalke/bitcoin that referenced this pull request Sep 27, 2018

Merge bitcoin#14316: tests: exclude all tests with difference paramet…
…ers in `--exclude` list

c7b3e48 tests: exclude all tests with difference parameters (Chun Kuan Lee)

Pull request description:

  Fix broken exclusion list in functional tests. See bitcoin#14007 (review)

Tree-SHA512: b6c2b86fef13e3c00c695adaeeb3e47ee9b48877c71bc605d24201ce931b2ef3ae9f5f199071fa1ec5de2d7aadc478410094c380cc297922e683e9b2569cda03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.