-
Notifications
You must be signed in to change notification settings - Fork 39
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
Added new testing facilities. #125
Conversation
…ands autoamtically when configured to do so.
lib/membrane/integration/testing_configurable_pipeline/assertions.ex
Outdated
Show resolved
Hide resolved
lib/membrane/integration/testing_configurable_pipeline/assertions.ex
Outdated
Show resolved
Hide resolved
Maybe before we merge this write some tests in some element (UDP?) using these utils to check if they work (I think writing tests for tests is a bit overkill as for now) |
import ExUnit.Assertions, only: [assert_receive: 1] | ||
|
||
quote do | ||
assert_receive {Membrane.Integration.TestingConfigurable.Pipeline, unquote(pattern)}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is no such module as Membrane.Integration.TestingConfigurable.Pipeline
but there is Membrane.Integration.TestingConfigurablePipeline
. A dot makes the difference.
import ExUnit.Assertions, only: [assert_receive: 1] | ||
|
||
quote do | ||
assert_receive {Membrane.Integration.TestingConfigurable.Pipeline, unquote(pattern)}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As for the name itself, I really hate the current naming, it reminds me of Java and that's not a good thing. I would remove Integration
instead and go for something like Membrane.Testing.ConfigurablePipeline
or with Test
instead of Testing
@mat-hek
import ExUnit.Assertions, only: [assert_receive: 1] | ||
|
||
quote do | ||
assert_receive {Membrane.Integration.TestingConfigurable.Pipeline, unquote(pattern)}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And one more: why ConfigurablePipeline
? Is there a non-cofigurable version available for users? This one should be called TestingPipeline to be consistent with elemets in that module. Of course, that would mean the TestingPipeline
from support should be renamed, but it will be now test-specific and I would rename it to sth like Membrane.DemandTest.(Testing)Pipeline
These elements are based on what I wrote for UDP element |
36993c3
to
ac67869
Compare
Fix the build first https://travis-ci.com/membraneframework/membrane-core/builds/94304322 |
lib/membrane/testing/pipeline.ex
Outdated
monitored_callbacks: pipeline_callback(), | ||
test_process: pid(), | ||
elements: Spec.children_spec_t(), | ||
links: Spec.links_spec_t() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not enforced keys can be nil as well
lib/membrane/testing/pipeline.ex
Outdated
Structure representing `options` passed to testing pipeline. | ||
|
||
## Monitored Callbacks | ||
List of callback names that shall be sent to process in `test_process` field |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This sentence does not actually explain what it does. Process test_process
gets notified about the invocation of the callbacks passed here. Now description suggests the list will be sent at some point
- Fixed few typos in existing documentation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See unresolved comments from the previous review. Resolve conflicts
Added TestingConfigurablePipeline and modified test sink to place demands autoamtically when configured to do so.