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

Support for direct requireJs dependency #99

Closed
wants to merge 4 commits into from

Conversation

lmartorella
Copy link

Hi,
It would be a pleasure if you can review and discuss the proposed change.
Basically I've found that with the current API it is not possible to add "module" dependency to the QUnit runner. All the names passed in opts.deps will be parsed as JS filename and prepended with its full path.

However this makes impossible to add a node.js/requireJs module dependency to the runner.
I've tried adding a new opts.moduleDeps field that will not be parsed as javascript source file, but simply passed as-is to the _require function running in the child process.

Doing that it seems possible to declare module dependency. We are using it in order to load the "qunit-reporter-junit" module to obtain a valid XML result file in our Jenkins environment.

Could you please review it? I don't like much the double 'deps / moduleDeps' parameter but I cannot find something better at the moment.

In addition, I've made a couple of changes to support run without a valid "code" field. This can happen if, for example, the Typescript compiler is used to compile both QUnit tests and code: in that case we will obtain a single .js file that contains the library to test and the test cases.
So basically with this fix we are able to test Typescript test case with QUnit directly using nodeJs as engine and Jenkins as agent.

Thank you,
Luciano

@kof
Copy link
Contributor

kof commented Sep 3, 2014

hey, thanks for taking time.

  1. deps and moduleDeps is not nice. We can make deps support module names too. Also if you run your code on server only you can use require directly in the child.
  2. why do you compile typescript tests and code in one file, how about to separate them? I mean its not nice to do that in general, no?
  3. so you are using qunit-reporter-junit inside of child to generate junit conform results output? Never tried it and we need to support it for QUnit addons cannot be used #100 anyways.

So lets work on those 3 issues separately, ok?

@lmartorella
Copy link
Author

Hi kof,
Sure, do you prefer different pull requests be created for these items?

Re about Typescript, it is possible to produce separated JS file only when working with modules. However, when developing UTs it is common to test single 'internal' classes of a library. Requiring these classes to be exported in a module could be overkill and not clean for the final API.
So basically we are trying to create different unit-test files for each part of the library. This forces the Typescript compiler to produce a single .js file for each test, that include the code at the top.

In addition, for sake of simplicity, allowing empty 'code' options let you to write sample UT files as documentation that doesn't require an external code to be written (like in other languages).

Re your 1. and 3. points, yes, we are able to load the "qunit-reported-junit" module (right now a fork, to fix a little nodeJs incompatibility issue) right before our test JS code, using the "moduleDeps" hack. The produce XML files is fed to Jenkins and nice reports are now available in our builds.

I think the main issue is about the automatic absolute path expansion put in place for the "deps" options, regardless their nature (local JS file to the testing module, or external module).
Another viable solution perhaps could be putting a try/catch block in the _require function, first trying without the path expansion, and then -when it fails- adding the absolute path.
This will fix the ugly 'deps/moduleDeps' API.

Thank you,
Luciano

@kof
Copy link
Contributor

kof commented Sep 5, 2014

I will create 3 separate issues and close this pull, ok?

@kof kof closed this Sep 5, 2014
@kof
Copy link
Contributor

kof commented Sep 5, 2014

@kof
Copy link
Contributor

kof commented Sep 5, 2014

please feel free to send me separate pulls.

I saw already you messed up tabs and spaces and saw wrong formatting. I suppose the current pull request is a quick draft so I don't comment on details.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants