feat(cli): Added url hash to differentiate between urls #835
Conversation
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
jaspritt-hora
force-pushed
the
adding-url-hash-report-generation
branch
from
6c0677f
to
8f4e505
Compare
|
@patrickhulce - Changes are done. Please review and let me know, if anything is required. |
packages/cli/src/upload/upload.js
Outdated
| @@ -542,7 +542,7 @@ async function runFilesystemTarget(options) { | |||
| const fetchTimeDate = new Date(new Date(lhr.fetchTime).getTime() || Date.now()); | |||
| const context = { | |||
| hostname: url.hostname, | |||
| pathname: url.pathname, | |||
| pathname: url.hash + url.pathname, | |||
There was a problem hiding this comment.
we don't want to merge these two, we would want them to be separate
| pathname: url.hash + url.pathname, | |
| pathname: url.pathname, | |
| hash: url.hash, |
There was a problem hiding this comment.
@patrickhulce - Regarding the test case, we want to add the test case for both collect & upload or just the latter?
There was a problem hiding this comment.
Collect isn't being touched here, just upload
There was a problem hiding this comment.
@patrickhulce - Sounds good! I have made the suggested change and written the test case in a new file and it is working as expected.
However, I have a question, NOT all the test cases are running on my machine.
Refer following screenshot -
The unit test case file, I have written is - upload-url-hash.test.js. The test for this has passed.
However, NOT ALL the test suites are passing. Refer following screenshot -
Can I go ahead and commit the changes ?
OR
Do I need the entire test suite to pass?
There was a problem hiding this comment.
don't worry about the whole test suite for now, some are flaky and/or require other setup on your machine like mysql, if upload tests are passing you should be a-ok
There was a problem hiding this comment.
@patrickhulce - Please run all the checks again. The issue was related to syncing of directories, I have added rimraf and the required code for the same. The test case is running successfully now, had 3 consecutive successful runs.
There was a problem hiding this comment.
@patrickhulce - Checked the raw logs for both windows and macOS checks that have failed, in that - the test case written for the PR has passed successfully.
The test case file name is - upload-url-hash.test.js.
Let me know, if there anything actionable at my end.
jaspritt-hora
force-pushed
the
adding-url-hash-report-generation
branch
from
e1a151a
to
1f1a11a
Compare
jaspritt-hora
force-pushed
the
adding-url-hash-report-generation
branch
from
8086a00
to
fc8f7cf
Compare
| @@ -0,0 +1,113 @@ | |||
| /** | |||
| * @license Copyright 2019 Google Inc. All Rights Reserved. | |||
There was a problem hiding this comment.
| * @license Copyright 2019 Google Inc. All Rights Reserved. | |
| * @license Copyright 2022 Google Inc. All Rights Reserved. |
ADDED:
hashparameter, while generating reports for apps using #/NOTE: