- boostraping some mocha unit engine
- fixing lint
Details
Details
- Reviewers
joshuaspence epriestley - Group Reviewers
Blessed Reviewers
- write some mocha tests
- run arc unit
- go to party
Diff Detail
Diff Detail
- Repository
- rARC Arcanist
- Branch
- feature/mochaTestEngine
- Lint
Lint Errors Severity Location Code Message Error src/unit/engine/MochaTestEngine.php:60 XHP5 Use of Undeclared Variable Warning src/unit/engine/MochaTestEngine.php:60 XHP9 Naming Conventions Advice src/unit/engine/MochaTestEngine.php:117 XHP16 TODO Comment Advice src/unit/engine/MochaTestEngine.php:153 XHP16 TODO Comment - Unit
No Test Coverage - Build Status
Buildable 7700 Build 8437: [Placeholder Plan] Wait for 30 Seconds Build 8436: arc lint + arc unit
Event Timeline
Comment Actions
Still WIP, and it is my first real contribution.
I hope I respected https://secure.phabricator.com/book/phabcontrib/article/contributing_code/
I know your guys are busy doing better things but if you find some times commenting the idea of this patch and the writing, please do.
Comment Actions
I hope I respected https://secure.phabricator.com/book/phabcontrib/article/contributing_code/
The first section of this document is:
The most important parts of contributing code to Phabricator are:
- File a task with a bug report or feature request before you write code.
Can you help me understand why you didn't do that?
Comment Actions
I did speak of it in Z1336 and in irc and got no answer. So I diffed what I already did. Today I'm using it as extension in my project so don't worry, ever if it is trash for upstream it wont be lost time for me.
Basically I looked at the document later after diffing and take example of https://secure.phabricator.com/D12198 that haven't also a manifest task.
See irc log :
epriestley, i purpose D13949 without speaking to you first ? I just see this : "If you send us a patch without coordinating it with us first, it will probably be immediately rejected, or sit in limbo for a long time and eventually be rejected." Did this prevent me from beeing reviewed ?
Comment Actions
Changed title so interessed users can allaways patch it.
Sorry for disturbing upstream with this.
Comment Actions
Some inlines. Most importantly this should wait for T5568, after which I would expect the API to stabilize a bit.
| src/unit/engine/MochaTestEngine.php | ||
|---|---|---|
| 2 | There should be a blank line between <?php and any code, see D13534. | |
| 4 | Prefer to write this as [[https://mochajs.org/ | Mocha]]. | |
| 5 | Prefer to write this as [[https://gotwarlost.github.io/istanbul/ | Istanbul]] | |
| 12 | This isn't quite correct. @concrete-extensible only makes sense for non-final, non-abstract classes. | |
| 15–18 | See my next comment. | |
| 23–25 | Personally, I find this comment to be fairly useless and somewhat distracting. In this particular case, I don't think the comment provides any explanation that the code does not. | |
| 38–42 | IMO, this doesn't provide any value. | |
| 48–50 | As above. | |
| 54–56 | As aabove. | |
| 59–62 | As above. | |
| 65 | Why not just if ($this->getEnableCoverage()) { ? | |
| 73–75 | As above. | |
| 84 | As above. | |
| 104 | This should (later) come from arclint. | |
| 128–135 | I'm not sure about &&... particularly I'm not sure if this will work on Windows. | |
| 138 | This is equivalent to ExecFuture($cmd_line) | |
Comment Actions
Ty @joshuaspence. I will fix this and commit it again. No problem for waiting, I did it wrong with no speaking with core first so if it end up landing upstring, it will be party for me !!
Comment Actions
To be done :
- add unit tests (shall I ?? it will submit mocha to this repo if i do so since I assume mocha is not installed globally)
- fix nasty bugs (this currently bugs if there is no unit test or no line covered). I should allow having no unit tests or no covered lines, or at least prevent them from blocking arc unit
| src/unit/engine/MochaTestEngine.php | ||
|---|---|---|
| 38–42 | Will get rid of all this useless comments :) | |
| 65 | Indeed ! | |
| 104 | And get getConfigFromAnySource won't be froward compatible with .arclint and D13534 ? | |
| 128–135 | On production I have an other syntax that works on windows too. I will switch for this. | |