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 regression test harness upgrade proposal. #36

Merged
merged 2 commits into from
Aug 25, 2022

Conversation

jgiszczak
Copy link
Contributor

No description provided.


The current test harness hardcodes an initial listen port of 8888 on 127.0.0.1
and uses each port in sequence per node in the cluster for the http plugin.
The revised system will take advantage of the entirety of the 127.0.0.0/8
Copy link
Member

Choose a reason for hiding this comment

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

This many not be portable. I don't think macOS routes all of 127.0/8 by default, for example. How difficult will it be to have a fallback for such a platform that still executes sequentially?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wasn't aware macOS is not RFC 3330 compliant. macOS is not a supported platform. If and when we support it, this: sudo ifconfig lo0 alias 127.0.0.2 up can be added to the test harness when it's running on macOS. The Python platform module can be used to identify macOS as needed.

Copy link
Member

Choose a reason for hiding this comment

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

So are you saying for macos you can use a strategy that maps all IP & ports to 127.0.0.2:x?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, I wasn't clear. Variations of that sudo would be used for all IPs needed in the subnet. 127.0.0.x.

Copy link
Member

Choose a reason for hiding this comment

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

What will happen when a user runs ctest in this case?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

When a macOS user runs ctest they will get one password prompt on the command line the first time any test runs which requires the loopback addresses. All tests will check for their presence and silently use them if they're available, which will avoid additional password prompts that might otherwise arise because the sudo timeout has been reached before the entire suite of tests has completed.

- Execute code coverage tests in cooperation with CMake.
- Capture logs and other test artifacts into archives on commmand for upload to:
- Github test results;
- A status dashboard showing test results and code coverage statistics.
Copy link
Member

Choose a reason for hiding this comment

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

How/where will the dashboard be created?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ted requested it. Whether or not its data source is a Github action and where it would be hosted is yet to be determined.

@spoonincode
Copy link
Member

I do have one special request as part of the refactor: remove usage of curl in the python tests. There are a few usages of it currently, but it seems needless since python has built in HTTP client library

@stephenpdeos
Copy link
Member

A few notes from yesterday's discussion

Clarifications:

  • Most of this work is just a refactor of existing code written in python. The only true rewrite considered within this proposal is for eosio-launcher
  • Containerization via docker to help test run in parallel is not within scope because of its limited use by our assumed developer base
  • There are no major concern with how this scope of work intersects with @spoonincode 's ongoing efforts with build process, there will just need to be some degree of coordination and communication
  • The existing dfuse related tests were not considered within scope of this proposal, but warrant further discussion

Decisions:

  • Python is the correct language for this harness, namely because of its widespread use, flexibility, and that the codebase proposed to be refactored is already written in python
  • There should be additional robustness around transaction lifecycle added to this proposal
  • The supporting functions for setup and teardown of should be modularized out so it can be used across more use cases for various client test environments. This may not be required for an initial milestone for this initiative

@matthewdarwin
Copy link

Another point that was discussed is that the framework used to spin up a node(cluster) should be usable by downstream projects like eosjs.

@spoonincode
Copy link
Member

Yes, good to capture that requirement. It seems like the python script/scripts/module ought to be included in either the leap or leap-dev package.

@stephenpdeos stephenpdeos requested review from stephenpdeos and removed request for tedcahalleos August 24, 2022 21:03
Copy link
Member

@stephenpdeos stephenpdeos left a comment

Choose a reason for hiding this comment

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

Lgtm

@jgiszczak jgiszczak merged commit cf78749 into main Aug 25, 2022
@jgiszczak jgiszczak deleted the regression-test-harness-upgrades branch August 25, 2022 16:05
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.

4 participants