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

Introduce test prelude #53

Merged
6 commits merged into from
Aug 11, 2021
Merged

Introduce test prelude #53

6 commits merged into from
Aug 11, 2021

Conversation

ghost
Copy link

@ghost ghost commented Aug 9, 2021

This is a very simple PR that introduces a hydra-test-utils package which exposes a Hydra.Test.Prelude module and the Ports module previously from hydra-prelude.
I did not move code related to io-sim (from Test.Util) but I think it would fit in there.

@ghost ghost requested review from ch1bo and KtorZ August 9, 2021 15:35
@ch1bo
Copy link
Collaborator

ch1bo commented Aug 9, 2021

What's the rationale for another package? Wouldn't the hydra-prelude be part of each test component already?

@ghost
Copy link
Author

ghost commented Aug 9, 2021

Yes, but the converse is not true: Not all components that depend on hydra-prelude need to depend on test-related stuff like it was the case previously with Ports module. I think having less dependencies per component, and just the ones needed, is usually better as it reduces the risk of conflicts. Also I have the habits of segregating test and prod dependencies, once again to reduce width and depth of dependency graphs, and of keeping small and specialised packages in order also to speed up build and test time: Having finer grained packages, of course when the dependency graph doesn't degenerate into a list, helps in building only what's needed for some part of the system.
If both you and @KtorZ think we should not have a separate package for this, I am fine changing the PR.

@github-actions
Copy link

github-actions bot commented Aug 9, 2021

Unit Test Results

  2 files  ±0  26 suites  ±0   1m 4s ⏱️ ±0s
81 tests ±0  81 ✔️ ±0  0 💤 ±0  0 ❌ ±0 

Results for commit 946fee5. ± Comparison against base commit 946fee5.

♻️ This comment has been updated with latest results.

Copy link
Collaborator

@ch1bo ch1bo left a comment

Choose a reason for hiding this comment

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

Thanks. Some minor comments on what we could name different or move on top of this.

src
exposed-modules:
Hydra.Test.Prelude
Hydra.Network.Ports
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could: I would expect all modules of a package called "test-utils" to include some Test in the module name. Furthermore, other packages used in tests typically start with Test (e.g. Test.QuickCheck) and maybe we could name the prelude then also Test.Hydra.Prelude?

Copy link
Author

Choose a reason for hiding this comment

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

Good point. I will rename the Ports module. I am fine withTest.Hydra.XXX prefix, will do that renaming too.

import Network.WebSockets (Connection, receiveData, runClient, sendBinaryData)
import Test.Hspec
import Test.Hspec.QuickCheck (prop)
import Test.QuickCheck (cover)
import Test.QuickCheck.Monadic (monadicIO, monitor, run)
import Test.Util (failAfter, failure)
import Hydra.ServerOutput (ServerOutput (InvalidInput, ReadyToCommit))
import Test.Util (failAfter)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could: failAfter would also be a good candidate for the Test.Prelude

We provide a dedicated package for tests in order to not add more
dependencies to production code when test dependencies are not
needed.
Also, replace use of expectationFailure with failure which provides
better error reporting.
And replace another definition of failAfter
@ghost ghost force-pushed the abailly-iohk/introduce-test-prelude branch from 5990450 to 63e5bdc Compare August 10, 2021 14:33
@ghost ghost merged commit 946fee5 into master Aug 11, 2021
@ghost ghost deleted the abailly-iohk/introduce-test-prelude branch August 11, 2021 08:47
This pull request was closed.
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.

2 participants