Skip to content

Conversation

royjacobson
Copy link
Contributor

Marked every test running over 10s as 'slow'.

All the tests together - 10:13, only the fast tests - 2:40.

@royjacobson royjacobson requested review from dranikpg and talbii August 21, 2023 12:33
@romange
Copy link
Collaborator

romange commented Aug 21, 2023

@royjacobson does this PR will exclude slow tests from running? And if yes, when will we run them?

@ashotland
Copy link
Contributor

ashotland commented Aug 21, 2023

Should at least run all tests on release IMO

@dranikpg
Copy link
Contributor

dranikpg commented Aug 21, 2023

@royjacobson does this PR will exclude slow tests from running? And if yes, when will we run them?

Doesn't look like it

Did we actually decide to decrease out regtest running times? They run every 3hrs and take 30mins, so we have a window of about 2.5hrs 🤔

I thought we wanted to tackle CI tests

@talbii
Copy link
Contributor

talbii commented Aug 21, 2023

true, but its probably worthwhile to also tackle regression tests because it's only a matter of time until they become too slow. maybe its a bit too early to establish such slow/fast framework though.

@@ -5,3 +5,5 @@ log_date_format = %Y-%m-%d %H:%M:%S
log_file_level=DEBUG
log_cli = true
asyncio_mode=auto
markers =
slow: marks tests as slow (deselect with '-m "not slow"')
Copy link
Contributor

Choose a reason for hiding this comment

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

So by default we don't run slow tests. Right?

@kostasrim
Copy link
Contributor

Doesn't look like it

Did we actually decide to decrease out regtest running times? They run every 3hrs and take 30mins, so we have a window of about 2.5hrs 🤔

That's actually a good catch. I think for reg-tests we should also now include '-m "not slow`. From my understanding, this disables all slow tests by default so you have to be explicit when to run those.

kostasrim

This comment was marked as resolved.

@kostasrim kostasrim self-requested a review August 22, 2023 10:15
@royjacobson
Copy link
Contributor Author

This doesn't disable them be default, it's just marking them for exclusion with the flag.

I would like to add the fast tests on the pre-merge CI runs and leave the regression tests as-is, but we don't have to decide it here :)

@royjacobson royjacobson merged commit 331e6a4 into main Aug 23, 2023
@royjacobson royjacobson deleted the mark_slow_tests branch August 23, 2023 10:04
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.

6 participants