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

Chore e2e tests for full transaction lifecycle after introducing sci callback #995

Conversation

kbeker
Copy link
Contributor

@kbeker kbeker commented Dec 5, 2018

Resolve #744
Resolve #970

I haven't find any better name for SCIBaseTest so if you guys will have any idea for better name just tell me :)

@kbeker kbeker added verification use case Additional Verification use case payment use case 'force payment' use case acceptance use case 'force subtask results' use case labels Dec 5, 2018
@kbeker kbeker self-assigned this Dec 5, 2018
@kbeker kbeker force-pushed the chore-e2e-tests-for-full-transaction-lifecycle-after-introducing-sci-callback branch from 18a5f06 to cc97225 Compare December 5, 2018 11:59
Copy link
Contributor

@rwrzesien rwrzesien left a comment

Choose a reason for hiding this comment

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

Generally ok, but a lot of minor stuff so I am marking as request changes to follow up on this one.

Copy link
Contributor

@Jakub89 Jakub89 left a comment

Choose a reason for hiding this comment

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

I spoke through email with Igor from Golem recently, and he said that there is a way to make those payment transaction x6 faster. This is what he wrote:

...
powyzsze kroki realizuje klasa GNTConverter, która znajduje się w SCI, więc może będzie łatwiej z niej skorzystać.
API GNTConvertera jest następujące:
0. Musisz mieć GNT na koncie, więc wykonujesz to po request_gnt_from_faucet
1. Wołasz convert z wartością, którą chcesz przekonwertować
2. Wołasz is_converting w pętli (co 15 sekund na przykład) aż zwróci False, wtedy konwersja będzie zakończona. Całość może potrwać ~5min dla świeżego konta i ~3min dla używanego.

Golem nie korzysta z tej klasy właśnie przez aktywne oczekiwanie i przez to, że implementacja jest lekko nieoptymalna i można uzyskać krótszy czas oczekiwania, ale zostawiłem ją dla wygody do testów.
I dla zwiększenia szybkości testów na testnecie można sobie nadpisać stałą REQUIRED_CONFS na 1, wtedy wszystkie transakcje będą potwierdzone 6x szybciej, więc oczekiwania będzie dużo mniejszy :)

concent_api/api_testing_common.py Outdated Show resolved Hide resolved
concent_api/sci_testing_common.py Outdated Show resolved Hide resolved
@cameel
Copy link
Contributor

cameel commented Dec 7, 2018

@Jakub89

I spoke through email with Igor from Golem recently, and he said that there is a way to make those payment transaction x6 faster. This is what he wrote:

...
powyzsze kroki realizuje klasa GNTConverter, która znajduje się w SCI, więc może będzie łatwiej z niej skorzystać.
API GNTConvertera jest następujące:
0. Musisz mieć GNT na koncie, więc wykonujesz to po request_gnt_from_faucet
1. Wołasz convert z wartością, którą chcesz przekonwertować
2. Wołasz is_converting w pętli (co 15 sekund na przykład) aż zwróci False, wtedy konwersja będzie zakończona. Całość może potrwać ~5min dla świeżego konta i ~3min dla używanego.

Golem nie korzysta z tej klasy właśnie przez aktywne oczekiwanie i przez to, że implementacja jest lekko nieoptymalna i można uzyskać krótszy czas oczekiwania, ale zostawiłem ją dla wygody do testów.
I dla zwiększenia szybkości testów na testnecie można sobie nadpisać stałą REQUIRED_CONFS na 1, wtedy wszystkie transakcje będą potwierdzone 6x szybciej, więc oczekiwania będzie dużo mniejszy :)

Sounds like a good idea (well, as long as orphaned blocks are not common on the testnet; I'll take Igor's word for that). This would require setting SCIImplementation.REQUIRED_CONFS when we create an instance of the class. This is a class attribute (not instance attribute) so changes to this attribute are global. That should not be a problem as long as we set it before first use and no other part of the code changes it. But please make sure that SCI spawns its thread after we change the value. Otherwise it might be using the wrong one.

We could add a setting like OVERRIDE_REQUIRED_BLOCK_CONFIRMATIONS. By default set to None. If it's anything else than None, use its value to initialize REQUIRED_CONFS. In development.py and testing.py could be set to 1.

@dybi If you think it's a good idea, we could create an issue for this.

@kbeker kbeker force-pushed the chore-e2e-tests-for-full-transaction-lifecycle-after-introducing-sci-callback branch from d7efafb to 1bcc4c8 Compare December 11, 2018 13:06
@kbeker kbeker force-pushed the chore-e2e-tests-for-full-transaction-lifecycle-after-introducing-sci-callback branch 2 times, most recently from ade63c9 to bab8de3 Compare December 17, 2018 12:35
@cameel
Copy link
Contributor

cameel commented Dec 17, 2018

If the test cases in this pull request are based on examples from the #996 blueprint, please note that the expected results might change a little. You may need to update the pull request or create another after the blueprint is updated.

@kbeker kbeker force-pushed the chore-e2e-tests-for-full-transaction-lifecycle-after-introducing-sci-callback branch 5 times, most recently from cf06634 to 8b46fda Compare December 19, 2018 11:55
@kbeker kbeker force-pushed the chore-e2e-tests-for-full-transaction-lifecycle-after-introducing-sci-callback branch from 8b46fda to 5a43dd0 Compare December 19, 2018 12:34
@kbeker
Copy link
Contributor Author

kbeker commented Dec 19, 2018

@cameel

If the test cases in this pull request are based on examples from the #996 blueprint, please note that the expected results might change a little. You may need to update the pull request or create another after the blueprint is updated.

This PR doesn't change logic at all. I think any changes to tests logic should be done in different PR.

File with all necessary tools to create requestor's and provider's
account and deposit tokens into theirs accounts.
Private and public keys needs to be changed because now keys are
dependent on accounts which are given from SCIBaseTest
@kbeker kbeker force-pushed the chore-e2e-tests-for-full-transaction-lifecycle-after-introducing-sci-callback branch 3 times, most recently from 5460151 to 486fd1e Compare December 20, 2018 10:09
@kbeker kbeker force-pushed the chore-e2e-tests-for-full-transaction-lifecycle-after-introducing-sci-callback branch from 486fd1e to db81c5d Compare December 20, 2018 10:35
@kbeker kbeker merged commit db81c5d into master Dec 20, 2018
@kbeker kbeker deleted the chore-e2e-tests-for-full-transaction-lifecycle-after-introducing-sci-callback branch December 20, 2018 10:36
@kbeker kbeker added this to the next milestone Feb 20, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
acceptance use case 'force subtask results' use case payment use case 'force payment' use case verification use case Additional Verification use case
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants