Remove test-api-feature from chain-time #376

danielSanchezQ · 2020-05-11T10:42:52Z

Another module with support in the new chain-test-utils.

chain-test-utils/src/time/mod.rs

mzabaluev · 2020-05-11T12:39:14Z

chain-test-utils/src/time/mod.rs

+    gen().into()
+}
+
+// Generate an `Epoch` given a `smoke::R` (random generator)


Are we now transitioning to smoke from quickcheck, which also has a framework for arbitrary, but reproducible stuff? Does it add value to use both?

I wish I was able to see what smoke can do from the documentation, but there's scarcely any.

Well, the thing with quickcheck::Arbitrary is that since it is a trait it should be implemented in the same crate as the target type. So, the idea here is to have a "centralize" crate with the utilities to generate our inner types. That way that crate can be simply used in the tests without relying on features. We can still use quickcheck::Arbitrary for the self crate tests since it is useful, but rely on chain-test-utils for whatever is not in that crate.

About smoke, it just has some trait for generating types and abstracts some of the most basic generation of numbers.

So, the idea here is to have a "centralize" crate with the utilities to generate our inner types.

That might prove a bit more difficult to maintain in separation from the crates defining the types.

That way that crate can be simply used in the tests without relying on features.

What was the original problem we are trying to solve here? The "property-test-api" feature added the testing trait impls and possibly other test utilities for testing uses, and it was to be used in approximately this way:

# For non-test binaries. # If the whole target is meant for testing, add "property-test-api" here as well [dependencies] chain-core = { path = "chain-deps/chain-core" } # For cfg(test), integration tests under tests/, etc. [dev-dependencies] chain-core = { path = "chain-deps/chain-core", features = "property-test-api" }

@mzabaluev the whole point of a generator based approach, is that it allow to define those generator elsewhere, since they are effectively just function/closures. A generator is just an "infinite" iterator of random value where the random engine is deterministically generated from a given seed, hence assuring reproducibility from a given Seed.

I think I'm not explaining it properly, sorry.
With the property-test-api we are exporting the quickcheck dep. We are using this feature in the jormungandr tests (can't point out where exactly now). Which apparently is a misuse.

I see your point though. We could keep the feature for testing just the chain-deps. (But we may have the temptation in the future to use it just because it may be easier 🍔 )

So there are two things:

we want a more flexible API than arbitrary to construct testing data;

there is a concern about test facilities embedded into the library crates and conditionally enabled, as being cumbersome to use, or easy to get wrong and pull in unnecessary dependencies.

Would the latter be a matter of convention that has been followed only loosely, which may be what is causing problems?

Another benefit of testing APIs built into crates is that you don't have to break encapsulation to provide the testing facilities: a public generator defined in a conditionally compiled testing submodule can freely use the type's internals to generate values, while we don't want to provide equivalent freedom to construct values in public API.

Another benefit of testing APIs built into crates is that you don't have to break encapsulation to provide the testing facilities: a public generator defined in a conditionally compiled testing submodule can freely use the type's internals to generate values, while we don't want to provide equivalent freedom to construct values in public API.

Yes, this is something that will happen for sure.

Well, we can keep both though. And careful use them where they are needed.

chain-test-utils/src/utils/mod.rs

Fix time era test (missing including end value)

danielSanchezQ · 2020-05-12T09:22:43Z

I'm letting the feature-test-api as commented (bad branch name now). We can keep the testing utilities and see how to move in the future.

mzabaluev · 2020-05-12T09:27:45Z

chain-test-utils/src/time/mod.rs

+
+// `TimeEra` configuration, encapsulates the building boundaries for the inner data
+#[derive(Clone)]
+pub struct TimeEraGenCfg {


Maybe a bit less abbreviated: TimeEraGenConfig

mzabaluev · 2020-05-12T09:30:01Z

chain-test-utils/src/time/mod.rs

+    pub fn new(config: Option<TimeEraGenCfg>) -> Self {
+        Self { config }
+    }
+
+    pub fn with_config(&mut self, config: TimeEraGenCfg) {
+        self.config = Some(config);
+    }


These look a bit redundant: new could use the default configuration, so just be the same as the Default impl.

The TimeEraGenCfg struct looks like it could provide a builder-style API, but maybe it's too much polish for the testing interface.

mzabaluev · 2020-05-12T09:30:20Z

chain-test-utils/src/time/mod.rs

+        self.config = Some(config);
+    }
+
+    pub fn clear_config(&mut self) {


Do we really need the mutability?

Uhm, for modifying the struct I think we need.

mzabaluev · 2020-05-12T09:32:11Z

chain-test-utils/src/time/mod.rs

+}
+
+// Generate an `TimeEra` given a `smoke::R` (random generator) and a `TimeEraGenCfg` range limit tuple
+pub fn generate_time_era_with_config(r: &mut R, config: &TimeEraGenCfg) -> TimeEra {


This (as a public API) also looks redundant to the composition of TimeEraGenerator::with_config and the gen method.

Well, my idea was that some times you will need a one shot time build and some times you will need a bunch of generated items. It feels overkill to build a generator just to get one object when you can just call a method.

mzabaluev · 2020-05-12T09:38:04Z

chain-test-utils/src/time/mod.rs

+pub fn generate_slot(r: &mut R) -> Slot {
+    generate_slot_with(|| r.num())
+}


Can this generate nonsensical slot values?
I'm trying to understand the purpose of non-bounded random generator methods here.

Yes, it actually just makes sense if you want to test both acceptable values and not.

mzabaluev · 2020-05-12T09:39:14Z

chain-test-utils/src/time/mod.rs

+
+// Generator wrapper for TimeEra generator methods
+pub struct TimeEraGenerator {
+    config: Option<TimeEraGenCfg>,


I'd rather have a non-optional config and set range bounds to sensible defaults for a TimeEra.

Where can I find some documentation about what would be some range bounds for a TimeEra?

Good question. @vincenthz ?

I would like the elementary newtype values to be domain-valid by construction, unless their domain really covers the entire underlying integer type.

NicolasDP

Can you add some documentation to the types and the functions. I am looking at what kind of type can be added and how they are suppose to be used/interact with each other.

Also ergonimically speaking, put the types at the top if possible. So we know what we are looking for and the impl at the bottom (before the tests mod though).

NicolasDP · 2020-05-12T09:42:42Z

chain-test-utils/src/time/mod.rs

+    pub fn new(config: Option<TimeEraGenCfg>) -> Self {
+        Self { config }
+    }


what's the point to have a new with an optional of something and then a with_config to set the config?

use a Config and a ConfigBuilder if you need it. Use Default too. This will make your types more ergonomic.

Well, I was thinking that we may want to change the way the generator generates values mid-way on some computation and still keep the reproducible step from the same seed. I see that with_config was an unfortunate name for this. It should be a set_config.

Refactor TimeEraGenerator constructors

danielSanchezQ · 2020-05-12T14:28:10Z

@NicolasDP , I expanded (and fixed) the documentation. But I'm not sure if it was in the way you intended. I think I did not understand properly what you meant about the types interaction.

danielSanchezQ · 2020-05-13T08:39:49Z

I'm closing this for now. It will be resumed in the future.

danielsanchezq added 8 commits May 8, 2020 16:07

Move arbitrary implementations to test block

eef6bb3

Time era generation sketch

7a0abfc

Implemented TimeEra, Epoch and Slot methods.

a900b1f

Implemented TimeEraGenerator

ce57b79

Added new utils function to generate an R

61733a9

Added tests

201a097

Removed direct smoke dependency in mockchain

b247b18

Removed commented code

9e9096f

danielSanchezQ added the breaking-change label May 11, 2020

danielSanchezQ self-assigned this May 11, 2020

danielSanchezQ requested review from vincenthz, dkijania and NicolasDP May 11, 2020 10:42

danielSanchezQ added test and removed breaking-change labels May 11, 2020

danielSanchezQ marked this pull request as ready for review May 11, 2020 11:03

mzabaluev reviewed May 11, 2020

View reviewed changes

danielsanchezq added 2 commits May 11, 2020 15:23

Method renaming

92dbe98

Fix time era test (missing including end value)

Removed unnecessary allow(non_snake_case)

744f021

danielSanchezQ requested a review from mzabaluev May 11, 2020 13:43

danielsanchezq added 2 commits May 11, 2020 16:11

Removed feature from Cargo files

6bb1fc7

Bring back feature-test-api

1645d86

mzabaluev reviewed May 12, 2020

View reviewed changes

NicolasDP reviewed May 12, 2020

View reviewed changes

danielsanchezq added 3 commits May 12, 2020 12:49

Rename TimeEraGenCfg -> TimeEraGenConfig

4fb2105

Refactor TimeEraGenerator constructors

Expand some method documentation

17d3a64

Fix and expand docs with example

a1e4299

danielSanchezQ linked an issue May 12, 2020 that may be closed by this pull request

Generator based testing framework #378

Open

5 tasks

Specify R info in TimeEraGenerator doc

6412018

danielSanchezQ closed this May 13, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove test-api-feature from chain-time #376

Remove test-api-feature from chain-time #376

danielSanchezQ commented May 11, 2020 •

edited

Loading

mzabaluev May 11, 2020

danielSanchezQ May 11, 2020 •

edited

Loading

mzabaluev May 11, 2020

vincenthz May 11, 2020

danielSanchezQ May 11, 2020

mzabaluev May 11, 2020

danielSanchezQ May 11, 2020

danielSanchezQ commented May 12, 2020

mzabaluev May 12, 2020

mzabaluev May 12, 2020

mzabaluev May 12, 2020

mzabaluev May 12, 2020

danielSanchezQ May 12, 2020

mzabaluev May 12, 2020

danielSanchezQ May 12, 2020

mzabaluev May 12, 2020

danielSanchezQ May 12, 2020

mzabaluev May 12, 2020

danielSanchezQ May 12, 2020

mzabaluev May 12, 2020

NicolasDP left a comment

NicolasDP May 12, 2020

danielSanchezQ May 12, 2020

danielSanchezQ commented May 12, 2020

danielSanchezQ commented May 13, 2020

Remove test-api-feature from chain-time #376

Remove test-api-feature from chain-time #376

Conversation

danielSanchezQ commented May 11, 2020 • edited Loading

Choose a reason for hiding this comment

danielSanchezQ May 11, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danielSanchezQ commented May 12, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NicolasDP left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danielSanchezQ commented May 12, 2020

danielSanchezQ commented May 13, 2020

danielSanchezQ commented May 11, 2020 •

edited

Loading

danielSanchezQ May 11, 2020 •

edited

Loading