Clock: Time Extent, Maker, and Associated Traits by da2ce7 · Pull Request #83 · torrust/torrust-tracker

da2ce7 · 2022-09-12T23:34:56Z

TimeExtent is a simple structure that contains base increment (duration), and amount (a multiplier for the base the base increment). The resulting product of the base and the multiplier is the total duration of the time extent.

TimeExtentClock is a helper that generates time extents based upon the increment length and the time of the clock, this clock is specialised according to the test predicate with the DefaultClockExtentMaker type.

These modules will be used by the later connection cookie implementation.

da2ce7 · 2022-09-14T11:07:41Z

Rebased after merge of #81

mickvandijke · 2022-09-14T18:34:30Z

Where are the comments >:(

src/protocol/clock/period.rs

da2ce7 · 2022-09-19T06:11:22Z

To do: as per discussion rename from "period" to "interval" terminology.

20f099e refactor: move clock/clock.rs into clock.rs (clippy) (Cameron Garnham) Pull request description: Fix the clippy warning that the module has the same name as it's directory. Will recreate the folder when needed by the interval counter, i.e. torrust#83 ACKs for top commit: da2ce7: ACK 20f099e Tree-SHA512: 6a63d3a57fcb1a484d58ddd5ec2ebc868247939dbb73ac53e083bb0bfaff968bb5b4b322d46f83c8d005c53e1afa055bd84398f5f3c9ffc33656d78e2a86e941

josecelano

I've added more comments.

src/protocol/clock/clockperiod.rs

src/protocol/clock/period.rs

src/protocol/clock/extent.rs

src/protocol/clock/extentmaker.rs

da2ce7 · 2022-09-20T20:25:58Z

To do: as per discussion rename from "period" to "interval" terminology.

Hello @WarmBeer @josecelano,

I have rethought the period structure, and now I think that I have a naming that is good. I have decided on the name of 'extent', while it is already used in computer science (for a continuous range of blocks on a disk), it is somewhat adoptable, as blocks all have the same size.

By using simple mathematical terms: Base, Multiplier, and Product for Extent Trait Types, and clear labels for the TimeExtent specialization: increment and amount. I hope the code is much more intuitively comprehensible.

I think that this pull request is now ready for review.

josecelano

@da2ce7 I've only added a couple of comments.

src/protocol/clock/timeextent.rs

josecelano · 2022-09-21T13:16:22Z

@da2ce7 these are the test for the new features:

        PASS [   0.009s] torrust-tracker protocol::clock::timeextent::test::fn_checked_duration_from_nanos::it_should_return_a_duration
        PASS [   0.009s] torrust-tracker protocol::clock::timeextent::test::fn_checked_duration_from_nanos::it_should_return_tryfrom_int_error
        PASS [   0.008s] torrust-tracker protocol::clock::timeextent::test::make_time_extent_now::it_should_return_a_time_extent
        PASS [   0.009s] torrust-tracker protocol::clock::timeextent::test::make_time_extent_now::it_should_return_none
        PASS [   0.286s] torrust-tracker protocol::clock::timeextent::test::make_time_extent_now::it_should_return_tryfrom_int_error
        PASS [   0.285s] torrust-tracker protocol::clock::timeextent::test::make_time_extent_now_after::it_should_return_a_time_extent
        PASS [   0.009s] torrust-tracker protocol::clock::timeextent::test::make_time_extent_now_after::it_should_return_none
        PASS [   0.009s] torrust-tracker protocol::clock::timeextent::test::make_time_extent_now_after::it_should_return_tryfrom_int_error
        PASS [   0.008s] torrust-tracker protocol::clock::timeextent::test::make_time_extent_now_before::it_should_return_a_time_extent
        PASS [   0.009s] torrust-tracker protocol::clock::timeextent::test::make_time_extent_now_before::it_should_return_none
        PASS [   0.008s] torrust-tracker protocol::clock::timeextent::test::time_extent_decrease::it_should_return_an_negitive_overflow
        PASS [   0.009s] torrust-tracker protocol::clock::timeextent::test::make_time_extent_now_before::it_should_return_tryfrom_int_error
        PASS [   0.008s] torrust-tracker protocol::clock::timeextent::test::time_extent_decrease::it_should_return_decreased
        PASS [   0.362s] torrust-tracker protocol::clock::timeextent::test::time_extent_from_sec::it_should_make_time_extent
        PASS [   0.370s] torrust-tracker protocol::clock::timeextent::test::time_extent_default::it_should_make_time_extent
        PASS [   0.009s] torrust-tracker protocol::clock::timeextent::test::time_extent_increase::it_should_postive_overflow
        PASS [   0.009s] torrust-tracker protocol::clock::timeextent::test::time_extent_increase::it_should_return_increased
        PASS [   0.008s] torrust-tracker protocol::clock::timeextent::test::time_extent_total::it_should_return_none
        PASS [   0.009s] torrust-tracker protocol::clock::timeextent::test::time_extent_new::it_should_make_time_extent
        PASS [   0.008s] torrust-tracker protocol::clock::timeextent::test::time_extent_total::it_should_return_total
        PASS [   0.008s] torrust-tracker protocol::clock::timeextent::test::time_extent_total::it_should_return_tryfrom_int_error
        PASS [   0.008s] torrust-tracker protocol::clock::timeextent::test::time_extent_total_next::it_should_get_the_time_extent_total_next
        PASS [   0.008s] torrust-tracker protocol::clock::timeextent::test::time_extent_total_next::it_should_return_none

When I'm reviewing or writing tests, I always check that reading this output makes sense without looking at the implementation. For me, in the case of tests, namespaces (mod names) are also important. For instance. In this case:

PASS [   0.009s] torrust-tracker protocol::clock::timeextent::test::fn_checked_duration_from_nanos::it_should_return_tryfrom_int_error

I would like to read:

PASS [   0.009s] torrust-tracker protocol::clock::timeextent::test::checked_duration_from_nanos_function::should_fails_when_nanos_are_out_of_the_duration_range

The concrete error is an implementation detail. I should not change the test name is you thrown a different error type. And with the new name I know when the function throws that error.

Same for other tests.

I like reading this test output like a documentation to know what the strcut/function/mod/ does not only to protect from regression bugs.

UPDATE:

And I would use snake case:

PASS [   0.009s] torrust-tracker protocol::clock::time_extent::test::checked_duration_from_nanos_function::should_fails_when_nanos_are_out_of_the_duration_range

Notice time_extent instead of timeextent.

da2ce7 · 2022-09-21T13:39:13Z

@josecelano reworded the last comments to include you as the co-author. :)

josecelano

@da2ce7 wonderful!!!

With a little bit of indent and order in the output of the tests, you get this beautiful documentation for the mod:

clock::time_extent::test::
    fn_checked_duration_from_nanos::
        it_should_be_the_same_as_duration_implementation_for_u64_numbers
        it_should_fail_for_numbers_that_are_too_large
        it_should_give_zero_for_zero_input
        it_should_work_for_some_numbers_larger_than_u64

    make_time_extent::
        fn_now::
            it_should_fail_for_zero
            it_should_give_a_time_extent
            it_should_fail_if_amount_exceeds_bounds

        fn_now_after::
            it_should_fail_for_zero
            it_should_fail_if_amount_exceeds_bounds
            it_should_give_a_time_extent

        fn_now_before::
            it_should_fail_for_zero
            it_should_fail_if_amount_exceeds_bounds
            it_should_give_a_time_extent
    
    time_extent::
        fn_increase::
            it_should_increase
            it_should_not_increase_for_zero
            it_should_fail_when_attempting_to_increase_beyond_bounds

        fn_decrease::
            it_should_decrease
            it_should_not_decrease_for_zero
            it_should_fail_when_attempting_to_decrease_beyond_bounds
            
        fn_default::
            it_should_default_initialize_to_zero

        fn_from_sec::
            it_should_make_from_seconds
            it_should_make_empty_for_zero

        fn_new::
            it_should_make_empty_for_zero
            it_should_make_new

        fn_total::
            it_should_be_zero_for_zero
            it_should_fail_when_product_is_too_large
            it_should_fail_when_too_large
            it_should_give_a_total

        fn_total_next::
            it_should_be_zero_for_zero
            it_should_fail_when_product_is_too_large
            it_should_fail_when_too_large
            it_should_give_a_total

We should write a script to format the output like that. Like other testing frameworks.

For example: https://github.com/Nautilus-Cyberneering/git-queue/actions/runs/3094430462/jobs/5007796148#step:4:6

src/protocol/clock/timeextent.rs

`TimeExtent` is a simple structure that contains base increment (duration), and amount (a multiplier for the base the base increment). The resulting product of the base and the multiplier is the total duration of the time extent. `TimeExtentMaker` is a helper that generates time extents based upon the increment length and the time of the clock, this clock is specialised according to the `test` predicate with the `DefaultClockTimeExtentMaker` type.

* Write tests for all functions. * Rename `now_add` to `now_after` and `now_sub` to `now_before`. * Rework time extent totals calculations to work with larger numbers.

Rename: from `DefaultClockTimeExtentMaker` to `DefaultTimeExtentMaker`, and the associated types. Co-authored-by: Jose Celano <[email protected]>

Rename: from timeextent to time_extent. Co-authored-by: Jose Celano <[email protected]>

da2ce7 · 2022-09-22T07:54:30Z

ACK e785a7f

da2ce7 mentioned this pull request Sep 13, 2022

Udp: Basic Connection Cookie Implementation #85

Merged

da2ce7 force-pushed the period-clock branch from 400ea20 to cdbe0d1 Compare September 14, 2022 10:57

da2ce7 requested review from josecelano and mickvandijke September 14, 2022 11:08

mickvandijke reviewed Sep 14, 2022

View reviewed changes

src/protocol/clock/period.rs Outdated Show resolved Hide resolved

mickvandijke reviewed Sep 14, 2022

View reviewed changes

src/protocol/clock/period.rs Outdated Show resolved Hide resolved

mickvandijke reviewed Sep 14, 2022

View reviewed changes

src/protocol/clock/period.rs Outdated Show resolved Hide resolved

mickvandijke mentioned this pull request Sep 14, 2022

Period clock changes (WIP) da2ce7/torrust-tracker#2

Closed

da2ce7 force-pushed the period-clock branch from cdbe0d1 to e7eba31 Compare September 15, 2022 07:36

da2ce7 mentioned this pull request Sep 19, 2022

refactor: move clock/clock.rs into clock.rs (clippy) #92

Merged

da2ce7 force-pushed the period-clock branch from d0d974e to 909f42e Compare September 20, 2022 16:26

da2ce7 changed the title ~~Clock: Periods~~ Clock: Extent and Extent Maker (From Clock) Sep 20, 2022

da2ce7 force-pushed the period-clock branch from 909f42e to cc93528 Compare September 20, 2022 16:44

josecelano reviewed Sep 20, 2022

View reviewed changes

da2ce7 force-pushed the period-clock branch 2 times, most recently from f5400b1 to e95b760 Compare September 20, 2022 20:18

da2ce7 changed the title ~~Clock: Extent and Extent Maker (From Clock)~~ Clock: Time Extent, Maker, and Associated Traits Sep 20, 2022

da2ce7 force-pushed the period-clock branch from e95b760 to c77bc86 Compare September 20, 2022 23:17

josecelano approved these changes Sep 21, 2022

View reviewed changes

src/protocol/clock/timeextent.rs Outdated Show resolved Hide resolved

src/protocol/clock/timeextent.rs Outdated Show resolved Hide resolved

src/protocol/clock/timeextent.rs Outdated Show resolved Hide resolved

da2ce7 force-pushed the period-clock branch from 6ce1d0b to cd78e4d Compare September 21, 2022 13:37

josecelano approved these changes Sep 21, 2022

View reviewed changes

src/protocol/clock/timeextent.rs Outdated Show resolved Hide resolved

src/protocol/clock/timeextent.rs Outdated Show resolved Hide resolved

da2ce7 added 2 commits September 21, 2022 20:17

time extent: add tests and some corrections

0cab30c

* Write tests for all functions. * Rename `now_add` to `now_after` and `now_sub` to `now_before`. * Rework time extent totals calculations to work with larger numbers.

da2ce7 and others added 3 commits September 21, 2022 20:17

time extent: rename TimeExtentMaker for clarity

8653f9a

Rename: from `DefaultClockTimeExtentMaker` to `DefaultTimeExtentMaker`, and the associated types. Co-authored-by: Jose Celano <[email protected]>

time extent: use snake_case for module name

3d743ed

Rename: from timeextent to time_extent. Co-authored-by: Jose Celano <[email protected]>

time extent: overhaul tests naming scheme

e785a7f

da2ce7 force-pushed the period-clock branch from 31a4aa0 to e785a7f Compare September 21, 2022 18:17

da2ce7 merged commit a049e29 into torrust:develop Sep 22, 2022

Conversation

da2ce7 commented Sep 12, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

da2ce7 commented Sep 14, 2022

Uh oh!

mickvandijke commented Sep 14, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

da2ce7 commented Sep 19, 2022

Uh oh!

josecelano left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

da2ce7 commented Sep 20, 2022

Uh oh!

josecelano left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

josecelano commented Sep 21, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

da2ce7 commented Sep 21, 2022

Uh oh!

josecelano left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

da2ce7 commented Sep 22, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

da2ce7 commented Sep 12, 2022 •

edited

Loading

josecelano commented Sep 21, 2022 •

edited

Loading

josecelano left a comment •

edited

Loading