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

UCOSP project - File coverage view #76

Closed
wants to merge 109 commits into
base: master
from

Conversation

Projects
None yet
5 participants
@LinkaiQi
Contributor

LinkaiQi commented Nov 28, 2017

File coverage

Show coverage for file at a given revision.

In order to show coverage information, file-coverage-view needs data from both hg and ActiveData:

Features:

  1. File-coverage-view highlights covered and uncovered line of a file.
  2. Show coverage percentage of the file with dynamic color.
  3. Show a number in each line represents the number of test-suite that cover the line.
  4. By default it shows all tests that cover the file in the RHS covered-tests-sidebar. If a line has been selected, the file-coverage-view will display which suite/chunk that covers the selected line with info of suite-name, chunk-number and platform.

PDF doc demo:

https://github.com/klahnakoski/firefox-code-coverage-frontend/blob/ucosp/docs/File_Coverage_View.pdf

Contributor:

Mentor: Kyle Lahnakoski (klahnakoski@mozilla.com)
Jennifer Yuen (jyuen1@ualberta.ca)
Linkai Qi (linkai@ualberta.ca)


This change is Reviewable

klahnakoski and others added some commits Oct 4, 2017

Merge pull request #49 from yuenj/ucosp
docs: Add prototype for viewing tests coverage for each line and file…
1. Add fileviewer
2. Using react-router to get revision and path variables from URL, and pass to fileviewer as Params
Issue 1: The fileviewer now is able to show file contents on the page
Given revision-number and the path, the fileviewer is able to fetch the sources from hg,
and displays it on the fileview.
Merge pull request #4 from LinkaiQi/fileviewer
fix the items I mentioned in other pull requests
Merge pull request #15 from yuenj/activedata
Overall, I see your intent, and it looks good.  Thank you!
Merge pull request #18 from LinkaiQi/fix-fileviewer-routing
Issue #5, fix routing on the fileviewer
Issue 16, simplify http call (refactoring)
Move http call funtion body(error handling, catch, json parser) to file_data.js
Bug fixing: fix bug caused by PR\#5 (extra slash in the 'path' variable)
Merge pull request #17 from LinkaiQi/ucosp-lin
Issue #16, simplify http call
Merge pull request #24 from yuenj/refactorFetchCalls
Merging this early to avoid merge conflicts.
Addressing PR/#27 review feedback
1. Using 'forEach' function to iterate array
2. Store 'test' obj directly, rather than an index to it
Merge pull request #27 from LinkaiQi/ucosp-dev
Excellent!  thank you!
@LinkaiQi

This comment has been minimized.

Show comment
Hide comment
@LinkaiQi

LinkaiQi Dec 17, 2017

Contributor

@yuenj will attempt to address current round of feedback. I think she will finish her last exam on Dec 21 (same as me, we are in the same university). There is a great chance that we are not able to complete is before Thursday.

Contributor

LinkaiQi commented Dec 17, 2017

@yuenj will attempt to address current round of feedback. I think she will finish her last exam on Dec 21 (same as me, we are in the same university). There is a great chance that we are not able to complete is before Thursday.

@yuenj

This comment has been minimized.

Show comment
Hide comment
@yuenj

yuenj Dec 17, 2017

Yes, my last exam is on the 20th. @LinkaiQi does 3009478 address any of Armen's new feedback?

yuenj commented Dec 17, 2017

Yes, my last exam is on the 20th. @LinkaiQi does 3009478 address any of Armen's new feedback?

@armenzg

This comment has been minimized.

Show comment
Hide comment
@armenzg

armenzg Dec 18, 2017

Collaborator

OK. I've decided to look at the PR even when not officially working.
I will follow up with you via email.

Collaborator

armenzg commented Dec 18, 2017

OK. I've decided to look at the PR even when not officially working.
I will follow up with you via email.

@yuenj

This comment has been minimized.

Show comment
Hide comment

yuenj commented Dec 21, 2017

@armenzg

This comment has been minimized.

Show comment
Hide comment
@armenzg

armenzg Dec 21, 2017

Collaborator

I will review the code in a bit, however, the most important thing to fix is this message:
"This branch cannot be rebased due to conflicts".

I assume you need to do this:

git remote add upstream git@github.com:mozilla/firefox-code-coverage-frontend.git
git pull upstream master

After you do that you will have to resolve all conflicts. If you're not comfortable at doing this you will have to do some tutorial and watch some videos. It is painful but it is part of getting the code ready to land.

Collaborator

armenzg commented Dec 21, 2017

I will review the code in a bit, however, the most important thing to fix is this message:
"This branch cannot be rebased due to conflicts".

I assume you need to do this:

git remote add upstream git@github.com:mozilla/firefox-code-coverage-frontend.git
git pull upstream master

After you do that you will have to resolve all conflicts. If you're not comfortable at doing this you will have to do some tutorial and watch some videos. It is painful but it is part of getting the code ready to land.

@armenzg

This comment has been minimized.

Show comment
Hide comment
@armenzg

armenzg Dec 21, 2017

Collaborator

I've deployed this branch to Heroku:
https://fccf-staging.herokuapp.com/#/file?revision=2d580aeac901&path=/accessible/atk/Platform.cpp

Before fixing any of my comments please make sure you fix the "branch cannot be rebased due to conflicts.". It's very important you focus on this first.

Also fix the following console error:

Warning: Each child in an array or iterator should have a unique "key" prop.
Check the render method of FileViewer. See https://fb.me/react-warning-keys
for more information.

You need to look into fixing this overrun:
image

Another thing I've noticed is that this text speaks of "lines added" when we're not looking at a diff:
image
Perhaps a better text would be "29 lines covered out of 134 coverable lines"

@klahnakoski f?
If I click on a white line I see "No test covers this line", however, when I click a red line I see all tests that cover this file.
Should white lines say "This is a line that is not coverable" and red lines say "There is no test that covers this line"?

There's some weirdness in here (We can do a follow up; let's get this PR landed):
image

Collaborator

armenzg commented Dec 21, 2017

I've deployed this branch to Heroku:
https://fccf-staging.herokuapp.com/#/file?revision=2d580aeac901&path=/accessible/atk/Platform.cpp

Before fixing any of my comments please make sure you fix the "branch cannot be rebased due to conflicts.". It's very important you focus on this first.

Also fix the following console error:

Warning: Each child in an array or iterator should have a unique "key" prop.
Check the render method of FileViewer. See https://fb.me/react-warning-keys
for more information.

You need to look into fixing this overrun:
image

Another thing I've noticed is that this text speaks of "lines added" when we're not looking at a diff:
image
Perhaps a better text would be "29 lines covered out of 134 coverable lines"

@klahnakoski f?
If I click on a white line I see "No test covers this line", however, when I click a red line I see all tests that cover this file.
Should white lines say "This is a line that is not coverable" and red lines say "There is no test that covers this line"?

There's some weirdness in here (We can do a follow up; let's get this PR landed):
image

@armenzg

This comment has been minimized.

Show comment
Hide comment
@armenzg

armenzg Dec 21, 2017

Collaborator

Reviewed 3 of 7 files at r2, 2 of 7 files at r3, 1 of 2 files at r4.
Review status: 10 of 14 files reviewed at latest revision, 18 unresolved discussions.


docs/File_Coverage_View.pdf, line 0 at r4 (raw file):
Please remove the .pdf and .jpeg files.
If you need these files for a follow up documentation PR remember to make a copy somewhere else.


src/style.css, line 180 at r4 (raw file):

.file-meta-ul {
	float: right;
  padding: 0;

Please fix the inconsistent indentation.


src/components/fileviewer.js, line 17 at r4 (raw file):

  }

  async componentDidMount() {

You might be surprised with this code but you can get rid of few "async" and "await" keywords:

armenzg@armenzg-mbp ccov_frontend$ git diff -U1
diff --git a/src/components/fileviewer.js b/src/components/fileviewer.js
index 285e03d..41764f6 100644
--- a/src/components/fileviewer.js
+++ b/src/components/fileviewer.js
@@ -16,5 +16,5 @@ export default class FileViewerContainer extends Component {

-  async componentDidMount() {
+  componentDidMount() {
     const { revision, path } = this.state;
-    await this.fetchData(revision, path, 'mozilla-central');
+    this.fetchData(revision, path, 'mozilla-central');
   }
@@ -30,3 +30,3 @@ export default class FileViewerContainer extends Component {

-  async fetchData(revision, path, repoPath = 'integration/mozilla-inbound') {
+  fetchData(revision, path, repoPath = 'integration/mozilla-inbound') {
     // Get source code from hg
@@ -42,3 +42,3 @@ export default class FileViewerContainer extends Component {
     try {
-      await Promise.all([fileSource(), coverageData()]);
+      Promise.all([fileSource(), coverageData()]);
     } catch (error) {

src/components/fileviewer.js, line 19 at r4 (raw file):

  async componentDidMount() {
    const { revision, path } = this.state;
    await this.fetchData(revision, path, 'mozilla-central');

When calling a function don't use a value that comes out of no where.
You can either create a constant at the top of the module (const REPOPATH = 'mozilla-central') OR have the function definition have a default value. In this case I prefer the later.


src/components/fileviewer.js, line 31 at r4 (raw file):

  }

  async fetchData(revision, path, repoPath = 'integration/mozilla-inbound') {

Always use 'mozilla-central' since all of our coverage data comes from that repository.


src/components/fileviewer.js, line 43 at r4 (raw file):

    // Fetch source code and coverage in parallel
    try {
      await Promise.all([fileSource(), coverageData()]);

Promise.all() will not continue until all items in the array are resolved, thus, not needed to await for it.
If we remove await in here then there is no more usage of await in the body of fetchData(), thus, we can remove async from its definition.
I know we have coverageData() and fileSource() within it, however, they're self-contained within the Promise.all()


src/components/fileviewer.js, line 145 at r4 (raw file):

    } else {
      msg = <span>&#x2714;</span>; // heavy checkmark
    }

I would do this in one line. Not a blocker:
const msg = (data) ? '&#x2714' : '&#x2026'; // Heavy checkmark of horizontal ellipsis
Or even:

const showStatus = (label, data) => (
   <li className="file-meta-li">
      {label}: {(data) ? '&#x2714' : '&#x2026'}  // Heavy checkmark of horizontal ellipsis
   </li>);

You can keep the span within the <li> even though I don't think there's need for one.


src/components/fileviewercov.js, line 11 at r1 (raw file):

Previously, armenzg (Armen Zambrano) wrote…

No need to set this. Since it does not represent a data structure and you actually manipulate it anywhere in the class.

I have not been able to figure out what this expandTest functionality is for. Would you mind removing it unless I'm missing something?


src/components/fileviewercov.js, line 75 at r1 (raw file):

  <li>
    <div className="toggleable-test-title" onClick={() => handleTestOnExpand(row)}>
      <span className={`test-ptr ${expand}`}>&#x2023;</span>

Could you please define this symbol as a constant at the top of the module and refer to it? ("magical string" issue)


Comments from Reviewable

Collaborator

armenzg commented Dec 21, 2017

Reviewed 3 of 7 files at r2, 2 of 7 files at r3, 1 of 2 files at r4.
Review status: 10 of 14 files reviewed at latest revision, 18 unresolved discussions.


docs/File_Coverage_View.pdf, line 0 at r4 (raw file):
Please remove the .pdf and .jpeg files.
If you need these files for a follow up documentation PR remember to make a copy somewhere else.


src/style.css, line 180 at r4 (raw file):

.file-meta-ul {
	float: right;
  padding: 0;

Please fix the inconsistent indentation.


src/components/fileviewer.js, line 17 at r4 (raw file):

  }

  async componentDidMount() {

You might be surprised with this code but you can get rid of few "async" and "await" keywords:

armenzg@armenzg-mbp ccov_frontend$ git diff -U1
diff --git a/src/components/fileviewer.js b/src/components/fileviewer.js
index 285e03d..41764f6 100644
--- a/src/components/fileviewer.js
+++ b/src/components/fileviewer.js
@@ -16,5 +16,5 @@ export default class FileViewerContainer extends Component {

-  async componentDidMount() {
+  componentDidMount() {
     const { revision, path } = this.state;
-    await this.fetchData(revision, path, 'mozilla-central');
+    this.fetchData(revision, path, 'mozilla-central');
   }
@@ -30,3 +30,3 @@ export default class FileViewerContainer extends Component {

-  async fetchData(revision, path, repoPath = 'integration/mozilla-inbound') {
+  fetchData(revision, path, repoPath = 'integration/mozilla-inbound') {
     // Get source code from hg
@@ -42,3 +42,3 @@ export default class FileViewerContainer extends Component {
     try {
-      await Promise.all([fileSource(), coverageData()]);
+      Promise.all([fileSource(), coverageData()]);
     } catch (error) {

src/components/fileviewer.js, line 19 at r4 (raw file):

  async componentDidMount() {
    const { revision, path } = this.state;
    await this.fetchData(revision, path, 'mozilla-central');

When calling a function don't use a value that comes out of no where.
You can either create a constant at the top of the module (const REPOPATH = 'mozilla-central') OR have the function definition have a default value. In this case I prefer the later.


src/components/fileviewer.js, line 31 at r4 (raw file):

  }

  async fetchData(revision, path, repoPath = 'integration/mozilla-inbound') {

Always use 'mozilla-central' since all of our coverage data comes from that repository.


src/components/fileviewer.js, line 43 at r4 (raw file):

    // Fetch source code and coverage in parallel
    try {
      await Promise.all([fileSource(), coverageData()]);

Promise.all() will not continue until all items in the array are resolved, thus, not needed to await for it.
If we remove await in here then there is no more usage of await in the body of fetchData(), thus, we can remove async from its definition.
I know we have coverageData() and fileSource() within it, however, they're self-contained within the Promise.all()


src/components/fileviewer.js, line 145 at r4 (raw file):

    } else {
      msg = <span>&#x2714;</span>; // heavy checkmark
    }

I would do this in one line. Not a blocker:
const msg = (data) ? '&#x2714' : '&#x2026'; // Heavy checkmark of horizontal ellipsis
Or even:

const showStatus = (label, data) => (
   <li className="file-meta-li">
      {label}: {(data) ? '&#x2714' : '&#x2026'}  // Heavy checkmark of horizontal ellipsis
   </li>);

You can keep the span within the <li> even though I don't think there's need for one.


src/components/fileviewercov.js, line 11 at r1 (raw file):

Previously, armenzg (Armen Zambrano) wrote…

No need to set this. Since it does not represent a data structure and you actually manipulate it anywhere in the class.

I have not been able to figure out what this expandTest functionality is for. Would you mind removing it unless I'm missing something?


src/components/fileviewercov.js, line 75 at r1 (raw file):

  <li>
    <div className="toggleable-test-title" onClick={() => handleTestOnExpand(row)}>
      <span className={`test-ptr ${expand}`}>&#x2023;</span>

Could you please define this symbol as a constant at the top of the module and refer to it? ("magical string" issue)


Comments from Reviewable

@armenzg

This comment has been minimized.

Show comment
Hide comment
@armenzg

armenzg Dec 21, 2017

Collaborator

Reviewed 3 of 7 files at r3, 1 of 2 files at r4.
Review status: all files reviewed at latest revision, 17 unresolved discussions.


src/utils/data.js, line 170 at r1 (raw file):

Previously, armenzg (Armen Zambrano) wrote…

Do the split('\n') here.

Jennifer you can do the split in here if you do this:

return (await res.text()).split('\n');

Comments from Reviewable

Collaborator

armenzg commented Dec 21, 2017

Reviewed 3 of 7 files at r3, 1 of 2 files at r4.
Review status: all files reviewed at latest revision, 17 unresolved discussions.


src/utils/data.js, line 170 at r1 (raw file):

Previously, armenzg (Armen Zambrano) wrote…

Do the split('\n') here.

Jennifer you can do the split in here if you do this:

return (await res.text()).split('\n');

Comments from Reviewable

@LinkaiQi

This comment has been minimized.

Show comment
Hide comment
@LinkaiQi

LinkaiQi Dec 24, 2017

Contributor

You need to look into fixing this overrun

How about adding a threshold width to display the TestsSideViewer? (solving the overrun issue: TestsSideViewer and FileViewer overlay each other)

  • If we don't have enough space in the right to show the TestsSideViewer (browseWindowsSize - fileViewWidth < TestsSideViewerWidth)
    displays message: "expand browser window to view test coverage"
  • Else:
    displays TestsSideViewer normally
Contributor

LinkaiQi commented Dec 24, 2017

You need to look into fixing this overrun

How about adding a threshold width to display the TestsSideViewer? (solving the overrun issue: TestsSideViewer and FileViewer overlay each other)

  • If we don't have enough space in the right to show the TestsSideViewer (browseWindowsSize - fileViewWidth < TestsSideViewerWidth)
    displays message: "expand browser window to view test coverage"
  • Else:
    displays TestsSideViewer normally
@armenzg

This comment has been minimized.

Show comment
Hide comment
@armenzg

armenzg Dec 24, 2017

Collaborator

The overrun can easily be fixed using CSS grids.
https://mozilladevelopers.github.io/playground/css-grid
Consider the page as two columns. The sidebar being one of them.

Feel free to follow this up on a new PR after this gets landed.
I would rather get this PR landed.

Merry Christmas.

Collaborator

armenzg commented Dec 24, 2017

The overrun can easily be fixed using CSS grids.
https://mozilladevelopers.github.io/playground/css-grid
Consider the page as two columns. The sidebar being one of them.

Feel free to follow this up on a new PR after this gets landed.
I would rather get this PR landed.

Merry Christmas.

Address code review feedbacks, includes:
1. fixed map function unique "key" issue
2. fixed lintes errors
3. css indentation And minor layout styling improvment
4. save symbols in a separate file "magical string issue"
@armenzg

This comment has been minimized.

Show comment
Hide comment
@armenzg

armenzg Jan 3, 2018

Collaborator

Reviewed 1 of 2 files at r5, 6 of 6 files at r6.
Review status: all files reviewed at latest revision, 3 unresolved discussions.


docs/File_Coverage_View.pdf, line at r4 (raw file):

Previously, armenzg (Armen Zambrano) wrote…

Please remove the .pdf and .jpeg files.
If you need these files for a follow up documentation PR remember to make a copy somewhere else.

Can you please remove the .jpeg and .pdf files?


src/components/fileviewercov.js, line 11 at r1 (raw file):

Previously, armenzg (Armen Zambrano) wrote…

I have not been able to figure out what this expandTest functionality is for. Would you mind removing it unless I'm missing something?

Could you please address this?


src/components/fileviewercov.js, line 75 at r1 (raw file):

Previously, armenzg (Armen Zambrano) wrote…

Could you please define this symbol as a constant at the top of the module and refer to it? ("magical string" issue)

I like how you solved putting magical strings into one module. Well done.


Comments from Reviewable

Collaborator

armenzg commented Jan 3, 2018

Reviewed 1 of 2 files at r5, 6 of 6 files at r6.
Review status: all files reviewed at latest revision, 3 unresolved discussions.


docs/File_Coverage_View.pdf, line at r4 (raw file):

Previously, armenzg (Armen Zambrano) wrote…

Please remove the .pdf and .jpeg files.
If you need these files for a follow up documentation PR remember to make a copy somewhere else.

Can you please remove the .jpeg and .pdf files?


src/components/fileviewercov.js, line 11 at r1 (raw file):

Previously, armenzg (Armen Zambrano) wrote…

I have not been able to figure out what this expandTest functionality is for. Would you mind removing it unless I'm missing something?

Could you please address this?


src/components/fileviewercov.js, line 75 at r1 (raw file):

Previously, armenzg (Armen Zambrano) wrote…

Could you please define this symbol as a constant at the top of the module and refer to it? ("magical string" issue)

I like how you solved putting magical strings into one module. Well done.


Comments from Reviewable

@armenzg

This comment has been minimized.

Show comment
Hide comment
@armenzg

armenzg Jan 3, 2018

Collaborator

Good job to all!

I’ve decided to get the code landed as a single commit (except the docs/ directory):
6713666

I would have preferred to let you deal with the merge conflicts, however, this will be easier.
Use the PR as a reference for all the work that went into this.

Can you please address my latest comments on follow up PRs (ignore the line about removing files from docs/)?:
#76 (comment)

Collaborator

armenzg commented Jan 3, 2018

Good job to all!

I’ve decided to get the code landed as a single commit (except the docs/ directory):
6713666

I would have preferred to let you deal with the merge conflicts, however, this will be easier.
Use the PR as a reference for all the work that went into this.

Can you please address my latest comments on follow up PRs (ignore the line about removing files from docs/)?:
#76 (comment)

@armenzg armenzg closed this Jan 3, 2018

@armenzg armenzg referenced this pull request Mar 1, 2018

Closed

Add docs about coverage #44

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