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

improve HTML - init commit #802

Closed

Conversation

vrozaev
Copy link
Contributor

@vrozaev vrozaev commented Sep 26, 2017

Hello @dylans, hello @jason0x43!

This pull request can fix several issues:
#636 #724

Now it's looks like this:
screenshot from 2017-09-25 20-13-23

Feedback:
npm test serveOnly command say Listening on localhost:9000 (ws 9001) it's not comfortable - maybe it's should say Listening on http://127.0.0.1:9000/__intern/ (ws 9001)? In that case you can copy-paste this URL to browser.

If you prefer arrow function for callback - maybe you should add linter rule for that?
https://eslint.org/docs/rules/prefer-arrow-callback

@vrozaev vrozaev force-pushed the improve-html-reporter-intern-4 branch from b45099f to 625db82 Compare September 26, 2017 08:08
@dylans dylans added this to the Intern 4.0 milestone Sep 26, 2017
@dylans dylans added the enhancement A new or improved feature label Sep 26, 2017
@@ -3,6 +3,32 @@ import Reporter, { eventHandler, ReporterProperties } from './Reporter';
import Test from '../Test';
import Suite from '../Suite';

const location = window.location;

function getFullName(test: Suite | Test): string {
Copy link
Member

@jason0x43 jason0x43 Sep 26, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't specify a return type when it's not necessary. Also applies to createLinkNode.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

let search = location.search;

if (search) {
search = search.slice(1).split('&').filter(function (el) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For consistency, prefer arrow functions for simple callbacks.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@jason0x43
Copy link
Member

I like where this is going. Some thoughts:

  • Use suites instead of grep (or both suites and grep if a specific test is being rerun) so that the only result in the re-run report will be the suite the user selected.
  • Allow the location to be passed into the reporter, like the document property. Add it to HtmlProperties and make it a reporter property, then move createLinkNode and getFullName into Html as methods, or allow the location to be passed into createLinkNode. This will improve testability.

@jason0x43
Copy link
Member

Also, the 'skipped' icon is rendering fine for me using Chrome on a Mac. Is there a particular browser you're seeing the issue with?

@vrozaev
Copy link
Contributor Author

vrozaev commented Sep 27, 2017

Also, the 'skipped' icon is rendering fine for me using Chrome on a Mac. Is there a particular browser you're seeing the issue with?

It was my mistake - I use outdated master branch.

@vrozaev vrozaev force-pushed the improve-html-reporter-intern-4 branch from 7f9aaa0 to 5ff87c5 Compare September 27, 2017 13:52
@vrozaev
Copy link
Contributor Author

vrozaev commented Sep 27, 2017

Allow the location to be passed into the reporter, like the document property. Add it to HtmlProperties and make it a reporter property, then move createLinkNode and getFullName into Html as methods, or allow the location to be passed into createLinkNode. This will improve testability.

Done, but now i need window.encodeURIComponent function. So, can I just add window property to the HtmlProperties or I should write some provider for window.encodeURIComponent?

Use suites instead of grep (or both suites and grep if a specific test is being rerun) so that the only result in the re-run report will be the suite the user selected.

@jason0x43 Are you sure about that?
Suites param is array of string like path/to/suite1.js, ...etc. Program suites has specific name, like bdd.describe("My first suite!", () => {});. And it's can be several program suites in single file.
I didn't understand how i can correlate them witch each other.

Maybe we should build another html representation in case when grep property is specified? Show skipped tests in another (last) section or something like that?

@vrozaev vrozaev force-pushed the improve-html-reporter-intern-4 branch 2 times, most recently from 13a6e78 to bc7badd Compare September 27, 2017 14:39
@vrozaev
Copy link
Contributor Author

vrozaev commented Sep 27, 2017

@jason0x43 one more question. I tried to write tests but looks like i should register me mocks somehow. What i should do? Stacktrace:

(ノಠ益ಠ)ノ彡┻━┻
Error: A plugin named "createLocation" has not been registered
  at Node.Executor.getPlugin  <src/lib/executors/Executor.ts:322:10>
  at <tests/unit/lib/reporters/Html.ts:6:31>
  at Object.defineProperty.value  <_tests/tests/unit/lib/reporters/Html.js:3:17>
  at Object.<anonymous>  <_tests/tests/unit/lib/reporters/Html.js:9:3>

@jason0x43
Copy link
Member

Assuming you want to use the same createLocation code in the browser and in Node, you don't need to deal with the plugin system, just do import { createLocation } from '../../../support/unit/mocks';. The setup used for DOM documents involves jsdom, and is a bit more complex than that to keep Intern from trying to load jsdom in browsers (or use the global document in Node).

@vrozaev vrozaev force-pushed the improve-html-reporter-intern-4 branch from e268dac to 890fc47 Compare September 28, 2017 19:19
@jason0x43
Copy link
Member

So, how's this going? From the last CI error, it looks like the self tests are just missing a few commas.

@vrozaev vrozaev force-pushed the improve-html-reporter-intern-4 branch from 890fc47 to 5dff892 Compare October 4, 2017 21:56
@codecov-io
Copy link

codecov-io commented Oct 4, 2017

Codecov Report

Merging #802 into master will decrease coverage by 0.02%.
The diff coverage is 85.71%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #802      +/-   ##
==========================================
- Coverage   92.96%   92.93%   -0.03%     
==========================================
  Files          54       54              
  Lines        4349     4361      +12     
  Branches      942      944       +2     
==========================================
+ Hits         4043     4053      +10     
- Misses        306      308       +2
Impacted Files Coverage Δ
src/lib/reporters/Html.ts 90.16% <85.71%> (-0.29%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 67ce151...4be0f00. Read the comment docs.

@vrozaev
Copy link
Contributor Author

vrozaev commented Oct 4, 2017

Well, I believe that feature itself is ready. There is only one little problem:

I can't just add test for suiteStart method because it's didn't actually add link anywhere. It's just store it in private property: this._runningSuites[suite.id] = { node: rowNode };. (I add test for testEnd method - now it's check that link for test exist.)

So - only one unit test left. Seems that I should call multiply methods if I want test this. It's sad - I was sure that I can call only suiteStart.

@jason0x43
Copy link
Member

You could aspect document.createElement for the reporter's document, and verify whether the expected elements were created or not, maybe something like:

import { after } from '@dojo/core/aspect';

// ...

test() {
    let nodes: HTMLElement[] = [];
    after(reporter.document, 'createElement', (returnValue: any, args: IArguments) => {
        nodes.push(returnValue);
        return returnValue;
    });

    // do test

    // make assertions about created nodes
}

@vrozaev vrozaev force-pushed the improve-html-reporter-intern-4 branch from 5dff892 to 4be0f00 Compare October 5, 2017 15:44
@vrozaev
Copy link
Contributor Author

vrozaev commented Oct 5, 2017

@jason0x43 It's ready now. Thanks for core/aspect - it's very useful.

@vrozaev
Copy link
Contributor Author

vrozaev commented Oct 16, 2017

@jason0x43 any news?

@jason0x43
Copy link
Member

Hah, good timing, I was just reviewing it.

@jason0x43 jason0x43 closed this in bec9d57 Oct 16, 2017
@jason0x43
Copy link
Member

I made a couple minor changes (updated the createLinkNode method to use @dojo/core/UrlSearchParams and styled the URLs to use the text colors). Thanks for your contribution!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement A new or improved feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants