Page MenuHomePhabricator

Add Mocha TestEngine (not worth review / ready for upstream yet)
AbandonedPublic

Authored by tycho.tatitscheff on Aug 20 2015, 8:06 PM.

Details

Reviewers
joshuaspence
epriestley
Group Reviewers
Blessed Reviewers
Summary
  • boostraping some mocha unit engine
  • fixing lint
Test Plan
  • write some mocha tests
  • run arc unit
  • go to party

Diff Detail

Repository
rARC Arcanist
Branch
feature/mochaTestEngine
Lint
Lint Warnings
SeverityLocationCodeMessage
Warningsrc/unit/engine/MochaTestEngine.php:94PHLXHP4Unsafe Usage of Dynamic String
Unit
No Test Coverage
Build Status
Buildable 7853
Build 8737: [Placeholder Plan] Wait for 30 Seconds
Build 8736: arc lint + arc unit

Event Timeline

tycho.tatitscheff retitled this revision from to Add mocha parser.
tycho.tatitscheff updated this object.
tycho.tatitscheff edited the test plan for this revision. (Show Details)

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.

tycho.tatitscheff retitled this revision from Add mocha parser to Add mocha test engine.Aug 20 2015, 10:16 PM

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?

epriestley edited edge metadata.
This revision now requires changes to proceed.Aug 24 2015, 5:16 PM

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?

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 ?

tycho.tatitscheff retitled this revision from Add mocha test engine to Add Mocha TestEngine (not worth review / ready for upstream yet).Aug 25 2015, 6:20 PM
tycho.tatitscheff edited edge metadata.

Changed title so interessed users can allaways patch it.
Sorry for disturbing upstream with this.

joshuaspence edited edge metadata.

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)

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 !!

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 ?
I will look forward to see the correct API.

128–135

On production I have an other syntax that works on windows too. I will switch for this.

tycho.tatitscheff edited edge metadata.
  • Fixing issues raised on review

Fixing most of the requested changes ;)

isfs removed a subscriber: isfs.
isfs added a subscriber: isfs.