-
-
Notifications
You must be signed in to change notification settings - Fork 6
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
Fix #2 - Allow to get other metrics #4
Fix #2 - Allow to get other metrics #4
Conversation
Looks good! I'll try to test this ASAP in the coming days. Ideally I would like to have some sort of automatic testing set up. Since it's quite time consuming to test this action manually 🤔 |
@emibcn this PR needs some refactoring due to the new changes. Could you please also add tests for this new feature? Thanks! |
….yml`, parse new argument, test, documentation.
c926d7b
to
9bfd5a1
Compare
….yml`, parse new argument, test, documentation. - v2 - Fix typo (missing `}`)
….yml`, parse new argument, test, documentation. - v3 - Make `$metricToParse` a real optional argument wich defaults to `elements``)
….yml`, parse new argument, test, documentation. - v3 - Fix indentation
….yml`, parse new argument, test, documentation. - v3 - Make `$metricToParse` a real optional argument wich defaults to `elements``), using only `$argv` to ensure tests work
….yml`, parse new argument, test, documentation. - v3 - Fix indentation and "tables"
….yml`, parse new argument, test, documentation. - v3 - Fix indentation and "tables" - v2
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.
Can you also add a MetricTest
feature test that proves it works?
Isn't that covered here? |
That is a unit test. So it tests the PHP code directly in the scope of the helper. Additionally, I would like to see it as a feature tests as well. So we also know it works in the context of the entire script being executed. So during the unit test we will know if the engine itself works, but I am also interested in knowing that the car itself will still run with that engine installed to make sure the engine is connected properly and is compatible with the car and the car actually moves forward. Did I explain that properly? |
…kflow pipeline
How about now? PS: Sorry about indentation commits: I don't use PHP since years ago and I don't have proper tooling installed. |
Yeah, very didactic ;) |
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.
Perfect, thank you very much!
Fixes #2