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

test: check hotreload #49

Closed
wants to merge 3 commits into from
Closed

test: check hotreload #49

wants to merge 3 commits into from

Conversation

ArtDu
Copy link
Contributor

@ArtDu ArtDu commented Apr 13, 2021

The class of problems that this test catches is the expirationd integration with cartridge. When hot-reloading roles (that is, when call require("cartridge.roles").reload() ), expirationd should exit gracefully. All fibers should terminate and the task list should be cleared.

These tests will also be used as an example of using hot reload and expirationd

tests:

  • check how expiraiond will behave when reloading a role without writing additional code in role stop
  • transactions will be processed correctly if we specify the use of batch transactions in expirationd.start
  • reloading the role with saving the iteration state and starting the expirationd task from the moment the last role ended

Closes #54

@ArtDu ArtDu requested a review from akudiyar April 13, 2021 16:47
@ArtDu ArtDu force-pushed the hotreload branch 3 times, most recently from 0448c89 to e4d7977 Compare April 20, 2021 17:39
@ArtDu ArtDu force-pushed the hotreload branch 9 times, most recently from 9087d91 to 6c7ff79 Compare May 14, 2021 17:06
@ArtDu ArtDu changed the title Added test for check hotreload Added test for check hotreload. Changed ci from travis to gh actions. May 14, 2021
Copy link

@akudiyar akudiyar left a comment

Choose a reason for hiding this comment

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

The changes must be validated with @Totktonada

@ArtDu ArtDu force-pushed the hotreload branch 6 times, most recently from df402a8 to fae1f29 Compare May 17, 2021 12:39
@ArtDu ArtDu changed the title Added test for check hotreload. Changed ci from travis to gh actions. Added test for check hotreload. Added gh actions. May 17, 2021
@ArtDu ArtDu changed the title Added test for check hotreload. Added gh actions. Test for check hotreload. Added gh actions for testing luatest. May 17, 2021
@ArtDu ArtDu requested a review from Totktonada May 20, 2021 07:48
@@ -0,0 +1,98 @@
local fio = require('fio')
Copy link
Member

Choose a reason for hiding this comment

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

What property of expirationd you verify here? I don't understand how all this code is relevant to expirationd.

Copy link
Member

Choose a reason for hiding this comment

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

There should be a well defined hypothesis about a class of problems we can catch this way (plus understanding why we can't do it in a more obvious and simple way) to actually add and support this testing code within the expirationd repository.

Copy link
Member

Choose a reason for hiding this comment

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

Please, take me right: I encourage using of smart testing techniques, where usual unit / functional testing are not powerful enough. Say, when a functional testing is able to catch particular set of highly specialized problems, but well implemented integration testing is able to catch almost any problem of given class. Or, say, the typical situation, when a functional testing unable to catch a real world problem, which only appears with data from a previous version of a module / an application.

I understand what cartridge/hotreload.lua doing (as well as moonlibs/module-reload). But I still don't catch the idea of this test. The questions that I ask for myself are:

  • What is the class of problems it should catch?

  • Whether it is beyond the class of problems that will be catched by the following code:

    local expirationd = require('expirationd')
    
    <..create a space..>
    <..fill the space..>
    expirationd.start(<...>)
    <..check all tuples are expired..>
    
    expirationd.kill(<...>)
    <..fill the space..>
    <..check that tuples are not expiring..>
    
    package.loaded.expirationd = nil
    
    expirationd = require('expirationd')
    expirationd.start(<...>)
    <..check all tuples are expired..>
  • Even more, whether it is beyoud problems we able to catch this way:

    require('expirationd')
    package.loaded.expirationd = nil
    require('expirationd')

Copy link
Contributor Author

Choose a reason for hiding this comment

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

in this case, @no1seman asked to make a test, which will clearly show that when call require("cartridge.roles").reload(), correct processing of the expirationd module is possible

@ArtDu ArtDu force-pushed the hotreload branch 3 times, most recently from b5d8ae3 to eecd305 Compare May 28, 2021 14:33
@ArtDu ArtDu changed the title Test for check hotreload. Added gh actions for testing luatest. [WIP] Test for check hotreload May 28, 2021
@ligurio ligurio self-requested a review June 15, 2021 12:45
If force is not specified, then it is now false by default, which is
indicated in the description. Before that, if force was not specified,
then it was correspondingly nil, which worked great, but was confusing
when it was impossible to find the force field in the task table
@ArtDu ArtDu force-pushed the hotreload branch 4 times, most recently from 1fb2066 to 49ed6aa Compare July 20, 2021 17:50
@ArtDu ArtDu added the tests label Jul 21, 2021
@ArtDu ArtDu changed the title [WIP] Test for check hotreload test: check hotreload Jul 21, 2021
@ArtDu ArtDu force-pushed the hotreload branch 2 times, most recently from 3b2a9ae to d546b79 Compare July 21, 2021 16:43
@ArtDu ArtDu force-pushed the hotreload branch 2 times, most recently from bca2c58 to 5575583 Compare July 24, 2021 14:08
The class of problems that this test catches is the expirationd
integration with cartridge. When hot-reloading roles
(that is, when call require("cartridge.roles").reload() ),
expirationd should exit gracefully.
All fibers should terminate and the task list should be cleared.

Also added cartridge rock to ci.
Bump luatest cause bug with before_suite.

Closes #54
@vrogach2020
Copy link

After discussion with @ligurio and @ArtDu there are some follow-ups:

  • split test cases into unit tests and integration tests
  • keep unit tests in this repo
  • if integration tests will require cartridge dependency probably we will move them to other repo (checking bundle integrations)

@ligurio
Copy link
Member

ligurio commented Nov 24, 2021

After discussion with @ligurio and @ArtDu there are some follow-ups:

So I'll close PR. Reopen it when changes will be ready to review.

@ligurio ligurio closed this Nov 24, 2021
@ligurio ligurio deleted the hotreload branch November 29, 2021 11:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add test for reloading the module with external iteration state via Cartridge hotreload
5 participants