Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

Clean test runner up #8485

Merged
merged 9 commits into from
Apr 7, 2021
Merged

Clean test runner up #8485

merged 9 commits into from
Apr 7, 2021

Conversation

seunlanlege
Copy link
Contributor

This just bumps parity-scale-codec in test-runner to v2

@seunlanlege seunlanlege added A2-insubstantial Pull request requires no code review (e.g., a sub-repository hash update). B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. labels Mar 29, 2021
@seunlanlege
Copy link
Contributor Author

@ordian @bkchr please take a look at this again

@seunlanlege
Copy link
Contributor Author

hey @kianenigma @gnunicorn could you please take a look at this?

@ordian ordian changed the title Bump scale-codec in test runner Clean test runner up Mar 31, 2021
@gnunicorn gnunicorn self-requested a review March 31, 2021 18:29
if let Some(base) = std::env::var("DB_BASE_PATH").ok() {
BasePath::new(base)
} else {
BasePath::new_temp_dir().expect("couldn't create a temp dir")
Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't this like a random path?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah, it is, if you don't supply an existing db path, it is assumed you want to test with a new db

Copy link
Contributor

Choose a reason for hiding this comment

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

yeah okay I was wondering if loading a fresh DB would make any sense, but yeah I presume you could also start with a new DB and it should be fine.

@seunlanlege
Copy link
Contributor Author

bump

1 similar comment
@seunlanlege
Copy link
Contributor Author

bump

@@ -71,6 +81,89 @@ impl ChainInfo for NodeTemplateChainInfo {
)
}

fn config(task_executor: TaskExecutor) -> Configuration {
Copy link
Contributor

Choose a reason for hiding this comment

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

why can't this stay in utils/test-runner? I was happy that in the previous PR you made it such, but why is it being reverted now?

I still think that 80% of the things being configured here are pretty irrelevant to the end user/programmer, thus not a good API.

Copy link
Contributor

Choose a reason for hiding this comment

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

At least we can provide different variations where you have fn config() as it is now, and a config_default() or something like this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There are cases where we'd like to run the test runner as a bin and provide it with command line args like a real node. (This is all simnet stuff), yeah i suppose a default would be nice 🤔

@ordian ordian merged commit a1574d3 into master Apr 7, 2021
@ordian ordian deleted the bump-codec branch April 7, 2021 12:54
hirschenberger pushed a commit to hirschenberger/substrate that referenced this pull request Apr 14, 2021
* bump scale-codec in test runner

* refactor config

* Update test-utils/test-runner/Cargo.toml

Co-authored-by: Andronik Ordian <write@reusable.software>

* bump cargo.lock

* add reasonable defaults

Co-authored-by: Andronik Ordian <write@reusable.software>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A2-insubstantial Pull request requires no code review (e.g., a sub-repository hash update). B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants