Skip to content

Conversation

@hebasto
Copy link
Member

@hebasto hebasto commented Sep 1, 2023

This PR aims to reduce the frequency of functional test failures on Windows like this one:


2023-09-01T01:05:01.850000Z TestFramework (ERROR): Assertion failed
Traceback (most recent call last):
  File "D:\a\bitcoin\bitcoin\test\functional\test_framework\test_framework.py", line 552, in start_nodes
    node.wait_for_rpc_connection()
  File "D:\a\bitcoin\bitcoin\test\functional\test_framework\test_node.py", line 296, in wait_for_rpc_connection
    self._raise_assertion_error("Unable to connect to bitcoind after {}s".format(self.rpc_timeout))
  File "D:\a\bitcoin\bitcoin\test\functional\test_framework\test_node.py", line 177, in _raise_assertion_error
    raise AssertionError(self._node_msg(msg))
AssertionError: [node 1] Unable to connect to bitcoind after 2400s

This code has had zero failures in my personal repository in more than 25 runs (and is still counting).


The second commit is a minor improvement to avoid "Cache save failed." warnings during job re-runs. For example:

image

@DrahtBot
Copy link
Contributor

DrahtBot commented Sep 1, 2023

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Reviews

See the guideline for information on the review process.

Type Reviewers
ACK MarcoFalke

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

@maflcko
Copy link
Member

maflcko commented Sep 1, 2023

The first commit will probably increase the run-time, but seems fine if it decreases the failure rate. Though, I don't understand why the failures would happen previously, maybe an OOM?

The second commit seems fine, but I think can also be dropped. The config is hard enough to parse already and I think a warning should be fine to ignore, but no strong opinion.

@hebasto
Copy link
Member Author

hebasto commented Sep 1, 2023

The first commit will probably increase the run-time

Yes, but insignificantly so.

Though, I don't understand why the failures would happen previously, maybe an OOM?

My best guess is that the reason is the flawed thread scheduler in the virtual environment, which might cause a new thread to stall.

The second commit seems fine, but I think can also be dropped. The config is hard enough to parse already and I think a warning should be fine to ignore, but no strong opinion.

I'd keep it. This commit follows the behavior of the combined actions/cache.

@hebasto
Copy link
Member Author

hebasto commented Sep 1, 2023

Updated 81806e2 -> f2d4e51 (pr28384.01 -> pr28384.02, diff):

  • restored accidentally omitted TEST_RUNNER_EXTRA

@hebasto
Copy link
Member Author

hebasto commented Sep 1, 2023

GHA CI is green. Going to restart it to demonstrate the benefits of the second commit.

@hebasto
Copy link
Member Author

hebasto commented Sep 1, 2023

Going to restart it to demonstrate the benefits of the second commit.

It is green again and clear of warnings as expected.

@MarcoFalke

Want to leave your final (I hope) comment here?

@maflcko
Copy link
Member

maflcko commented Sep 1, 2023

lgtm ACK f2d4e51 🐾

Show signature

Signature:

untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: lgtm ACK f2d4e510b37c0b132732ede214d29a8bf1daea93 🐾
iKbO5ixXRNdF9iEB+CfzJo/B1ZhUiZFKDkCDNCE9fHpfNOHeNpbpOGtH5up4V1e4O7teHxUn/a2AYpF0alPADw==

@glozow glozow merged commit b2f1d73 into bitcoin:master Sep 1, 2023
@hebasto hebasto deleted the 230831-gha-win branch September 1, 2023 12:56
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Sep 26, 2023
…n Windows

f2d4e51 ci: Avoid saving the same Ccache cache (Hennadii Stepanov)
14e5de6 ci: Avoid oversubscription in functional tests on Windows (Hennadii Stepanov)

Pull request description:

  This PR aims to reduce the frequency of functional test failures on Windows like this [one](https://github.com/bitcoin/bitcoin/actions/runs/6040229997):
  ```

  2023-09-01T01:05:01.850000Z TestFramework (ERROR): Assertion failed
  Traceback (most recent call last):
    File "D:\a\bitcoin\bitcoin\test\functional\test_framework\test_framework.py", line 552, in start_nodes
      node.wait_for_rpc_connection()
    File "D:\a\bitcoin\bitcoin\test\functional\test_framework\test_node.py", line 296, in wait_for_rpc_connection
      self._raise_assertion_error("Unable to connect to bitcoind after {}s".format(self.rpc_timeout))
    File "D:\a\bitcoin\bitcoin\test\functional\test_framework\test_node.py", line 177, in _raise_assertion_error
      raise AssertionError(self._node_msg(msg))
  AssertionError: [node 1] Unable to connect to bitcoind after 2400s
  ```

  This code has had zero failures in my personal repository in more than 25 runs (and is still counting).

  ---

  The second commit is a minor improvement to avoid "Cache save failed." warnings during job re-runs. For [example](https://github.com/bitcoin/bitcoin/actions/runs/5998688759):

  ![image](https://github.com/bitcoin/bitcoin/assets/32963518/d8a049df-fccd-4395-99c9-4be01d0ea706)

ACKs for top commit:
  MarcoFalke:
    lgtm ACK f2d4e51 🐾

Tree-SHA512: 0c92817d37325a114886900e49a4d644201397d98d6ac9f2dcd41170c7e7ea2cb1873f7e51b5cb3ad3cc2e59554ad1c8f87d439ea6c1c960bf5c339153be7040
@bitcoin bitcoin locked and limited conversation to collaborators Aug 31, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants