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

[sdk#898] Add clockmock Start #904

Merged

Conversation

Bolodya1997
Copy link

Description

  1. Add clockmock Start (needed for the discover stage in sandbox tests).
  2. Fix clocmock race issues.

Issue link

Relates to #898.

How Has This Been Tested?

  • Added unit testing to cover
  • Tested manually
  • Tested by integration testing
  • Have not tested

Types of changes

  • Bug fix
  • New functionallity
  • Documentation
  • Refactoring
  • CI

// Start starts mock time to run with the given speed until ctx becomes done. While time is running, current time for
// the mock will be the following:
// mock time := mock start time + (real time duration from the start) * speed + mock duration added with Set, Add
func (m *Mock) Start(ctx context.Context, speed float64) {
Copy link
Member

Choose a reason for hiding this comment

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

  1. Do we need stop method?
  2. What if Start has called twice?

Copy link
Author

Choose a reason for hiding this comment

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

  1. Nope, because we have ctx.
  2. Time will run with (speed-1 + speed-2) speed :)

Copy link
Member

Choose a reason for hiding this comment

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

Should this operation be thread-blockable?

Copy link
Author

Choose a reason for hiding this comment

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

Reworked with always running time and Mock.SetSpeed.

m.lock.RLock()
defer m.lock.RUnlock()

const tick = 10 * time.Millisecond
Copy link
Member

Choose a reason for hiding this comment

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

Should tick be configurable?

Copy link
Author

Choose a reason for hiding this comment

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

It can, but I can get no usecase why it should.

@Bolodya1997 Bolodya1997 self-assigned this May 6, 2021
@Bolodya1997 Bolodya1997 marked this pull request as draft May 6, 2021 04:49
Vladimir Popov added 2 commits May 14, 2021 12:07
Signed-off-by: Vladimir Popov <vladimir.popov@xored.com>
Signed-off-by: Vladimir Popov <vladimir.popov@xored.com>
@Bolodya1997 Bolodya1997 force-pushed the sdk#898/clock-run-time branch 2 times, most recently from c3b00b5 to e7a84fc Compare May 14, 2021 05:33
Signed-off-by: Vladimir Popov <vladimir.popov@xored.com>
@Bolodya1997 Bolodya1997 force-pushed the sdk#898/clock-run-time branch from e7a84fc to 05977b7 Compare May 14, 2021 05:47
@Bolodya1997 Bolodya1997 marked this pull request as ready for review May 14, 2021 05:52
@Bolodya1997 Bolodya1997 changed the title Add clockmock Start [sdk#898] Add clockmock Start May 14, 2021
@denis-tingaikin denis-tingaikin merged commit d09236b into networkservicemesh:main May 17, 2021
Mixaster995 pushed a commit to Mixaster995/sdk that referenced this pull request May 17, 2021
* Add clockmock Start

Signed-off-by: Vladimir Popov <vladimir.popov@xored.com>

* Fix race issues

Signed-off-by: Vladimir Popov <vladimir.popov@xored.com>

* Rework Mock.Start to Mock.SetSpeed

Signed-off-by: Vladimir Popov <vladimir.popov@xored.com>
Mixaster995 pushed a commit to Mixaster995/sdk that referenced this pull request May 17, 2021
* Add clockmock Start

Signed-off-by: Vladimir Popov <vladimir.popov@xored.com>

* Fix race issues

Signed-off-by: Vladimir Popov <vladimir.popov@xored.com>

* Rework Mock.Start to Mock.SetSpeed

Signed-off-by: Vladimir Popov <vladimir.popov@xored.com>
Signed-off-by: Mikhail Avramenko <avramenkomihail15@gmail.com>
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.

3 participants