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

pwmetrics is performing only a single round though three rounds were set #31

Closed
rpkoller opened this issue Jan 16, 2017 · 22 comments
Closed
Labels

Comments

@rpkoller
Copy link

Hi i am running pwmetrics 1.2.4 with lighthouse 1.4.1 on node 7.4 on osx 10.11.6. I am not sure why but somehow pwmetricsis only performing a single round of auditing instead of the three which i have set. :/

bildschirmfoto 2017-01-16 um 23 30 09 2
bildschirmfoto 2017-01-16 um 23 30 09

If you need further informations let me know. Best regards r.

@denar90
Copy link
Collaborator

denar90 commented Jan 17, 2017

@rpkoller thanks for using latest versions and helping us improve stuff! I'll try to use it with node v7 because with v6 I don't have any problems... It throws errors like yours but still run exact same number of runs was set.

@denar90 denar90 added the bug label Jan 20, 2017
@denar90
Copy link
Collaborator

denar90 commented Jan 26, 2017

@rpkoller I've tested with node 7.4.0 and osx 10.11.5. pwmetrics http://climbingflex.com --runs=3 works fine.
Did you close browser while metrics were getting or smth? Can you check it one more time?
Maybe it's happening because metrics were not available...

@rpkoller
Copy link
Author

rpkoller commented Jan 26, 2017

@denar90 I've reran the test. As far as i can remember there hasn't been any update to pwmetrics since the last time i've tried but oddly it is working now?!

bildschirmfoto 2017-01-26 um 13 33 02

Chrome was closed completely the last time and it was this time. each run a new window instance was opened. when the run was finished it closed. the first time a window opened only once this time three times. and in regards of your suspicion if the metrics weren't available at least a new window instance would have to pop up but it didnt the last time. quite odd and unsure how to narrow the root of the cause down. :/ Will try again later today if it gets back to the only one time measurement behavior. Strange. Is there some sort of "debug" attribute I might set for running pwmetrics showing what it is acutally doing?

@denar90
Copy link
Collaborator

denar90 commented Jan 26, 2017

Thanks for rechecking it 👍 We didn't have release yeat after 1.4.1, but master is far far away from this one. Taking a look at readme you can notice that.

Looks like some glitch but it will be nice to have couple tries to be confident in that.


We doesn't have any debug mode yet.
I'm using just node --inspect --debug-brk bin/cli.js

@rpkoller
Copy link
Author

yeah saw that. but i was just thinking out loud. couldn't imagine any other reason that one time the test is only ran once and then three times as given. but yeah will keep on testing.

on a side note, missing metrics can't be the reason that the test was only ran once in contrast to the given rounds. tried again right now an:

bildschirmfoto 2017-01-26 um 20 47 53

and thanks for the heads up in regard of inspect and debug. will give it a try later tonight. thanks!

This was referenced Feb 11, 2017
@denar90
Copy link
Collaborator

denar90 commented Feb 26, 2017

@rpkoller can you test with latest master?
Looks like issue was solved...

I tried same site as above with 5 runs. Looks good.

@rpkoller
Copy link
Author

@denar90 hm never installed a master from github with npm yet. pwmetricsi had installed globally with npm so far. now i've just created a directory and just tried npm install --save https://github.com/paulirish/pwmetrics/tarball/masterfrom within that directory. is that the correct way? cuz the version shows 1.2.4 would have expected 2.0? sorry for that probably trivial question. :/ gotta search a bit more tomorrow, now its already a bit too late, need some sleep.

@denar90
Copy link
Collaborator

denar90 commented Feb 26, 2017

I would use npm i --save paulirish/pwmetrics which bring latest changes from master.
pwmetrics --version will bring 1.2.4 because in package.json set this version. V 2.0 is not released yet.

@rpkoller
Copy link
Author

rpkoller commented Feb 26, 2017

@denart90 uh damn there seem some node 7.6 issues.yesterday nights install returned

Error: Cannot find module './sheets'
    at Function.Module._resolveFilename (module.js:470:15)
    at Function.Module._load (module.js:418:25)
    at Module.require (module.js:498:17)
    at require (internal/module.js:20:19)
    at Object.<anonymous> (/Users/myuser/npmtestballon/node_modules/pwmetrics/lib/index.js:13:16)
    at Module._compile (module.js:571:32)
    at Object.Module._extensions..js (module.js:580:10)
    at Module.load (module.js:488:32)
    at tryModuleLoad (module.js:447:12)
    at Function.Module._load (module.js:439:3)

on ./pwmetrics https://www.w3.org. thought it was due to the faulty way of installing. but did now like you said and replaced my current global pwmetrics install with the latest master with npm install -g --save paulirish/pwmetrics. when i tried pwmetrics --version afterwards to see if it is showing 2.0 now i got also :

module.js:472
    throw err;
    ^

Error: Cannot find module './sheets'
    at Function.Module._resolveFilename (module.js:470:15)
    at Function.Module._load (module.js:418:25)
    at Module.require (module.js:498:17)
    at require (internal/module.js:20:19)
    at Object.<anonymous> (/Users/myuser/.npm_global/lib/node_modules/pwmetrics/lib/index.js:13:16)
    at Module._compile (module.js:571:32)
    at Object.Module._extensions..js (module.js:580:10)
    at Module.load (module.js:488:32)
    at tryModuleLoad (module.js:447:12)
    at Function.Module._load (module.js:439:3)

happend with node 7.6 and npm 4.1.2 on osx :/

@denar90
Copy link
Collaborator

denar90 commented Feb 26, 2017

@rpkoller we are trying to migrate to typescript. So, to make it works in node_modules/pwmetrics just run npm run build.
Sorry, forgot to tell you about this.

@rpkoller
Copy link
Author

@denar90 no problem at all. :) but two followup question. I have installed pwmetrics globally with npm install -g --save paulirish/pwmetrics do i also have to run npm run buildand add -g so it becomes npm run -g build ... and where do i have to run that command? in the specific directory or is it applied to all installed npm packages and it is checked if a certain package needs building? and will it be necessary to run npm run build on each update from now on? or is it only necessary during that transitional phase?

@pedro93
Copy link
Collaborator

pedro93 commented Feb 26, 2017

@rpkoller first check that in your node_module's folder you have the pwmetrics project. If you do run the following command in that folder:

npm run build

If you don't have that pwmetrics folder, try and find the path where you global node packages are installed by running: npm root -g and running npm run build on the pwmetrics in that location. The -g flag does not apply to custom project dependant npm scripts.

@rpkoller
Copy link
Author

@pedro93 thanks. am not that npm savvy and still learning things. already found my way to the particular directory manually. but the npm root-grecommendation would have saved a bit of digging ;)))) went into the directory and ran npm run build... But again an error showed up :/

[myuser@olymp][~/.npm_global/lib/node_modules/pwmetrics]
$> npm run build

> pwmetrics@1.2.4 build /Users/myuser/.npm_global/lib/node_modules/pwmetrics
> tsc

sh: tsc: command not found

npm ERR! Darwin 15.6.0
npm ERR! argv "/usr/local/Cellar/node/7.6.0/bin/node" "/usr/local/bin/npm" "run" "build"
npm ERR! node v7.6.0
npm ERR! npm  v4.1.2
npm ERR! file sh
npm ERR! code ELIFECYCLE
npm ERR! errno ENOENT
npm ERR! syscall spawn
npm ERR! pwmetrics@1.2.4 build: `tsc`
npm ERR! spawn ENOENT
npm ERR!
npm ERR! Failed at the pwmetrics@1.2.4 build script 'tsc'.
npm ERR! Make sure you have the latest version of node.js and npm installed.
npm ERR! If you do, this is most likely a problem with the pwmetrics package,
npm ERR! not with npm itself.
npm ERR! Tell the author that this fails on your system:
npm ERR!     tsc
npm ERR! You can get information on how to open an issue for this project with:
npm ERR!     npm bugs pwmetrics
npm ERR! Or if that isn't available, you can get their info via:
npm ERR!     npm owner ls pwmetrics
npm ERR! There is likely additional logging output above.

npm ERR! Please include the following file with any support request:
npm ERR!     /Users/myuser/.npm_global/lib/node_modules/pwmetrics/npm-debug.log

maybe cuz it is still showing version 1.2.4 pwmetrics@1.2.4 (git://github.com/paulirish/pwmetrics.git#2f5a9b71769f6b02af36e6b6aafa71419662b452) as the installed version? even that i went with npm install -g --save paulirish/pwmetricsfor installation?

@pedro93
Copy link
Collaborator

pedro93 commented Feb 26, 2017

You learn something every day :D

The pwmetrics version is correct, it's what's specified here: https://github.com/paulirish/pwmetrics/blob/master/package.json. It looks like the typescript dependency is not installed when you call npm install ...

If you check the package json I linked too, typescript is set as dev dependency. Delete the pwmetrics folder and re-run the npm install but with the --dev flag:

$ npm install -g --dev --save paulirish/pwmetrics

@denar90
Copy link
Collaborator

denar90 commented Feb 26, 2017

The way I'm working with library.
Cloning repo git clone https://github.com/paulirish/pwmetrics && cd pwmetrics
then run either yarn or npm i
then npm link
Now on your machine latest master changes which are globally.

After next git pull run npm i && npm link. That's it :)

@rpkoller
Copy link
Author

@pedro93 ahhhh the version number is pulled from the package.json when i do a list of installed npm packages. cool and went then with the variant @denar90 is describing. one follow up question in that regard @denar90 . I haven't deleted the global package but followed his instructions. then i ran pwmetrics with 5 runs. i got the following output:

bildschirmfoto 2017-02-26 um 14 49 01

is it intentional that the graphs of each round arent shown anymore and just the median run? aside that i got no not available missing graphs like before and i used a page which is knowingly slow and had those issues. aside i got that invalid protexpectation. shall i file a separate issue for and do a trace?

@denar90
Copy link
Collaborator

denar90 commented Feb 26, 2017

@rpkoller it's a feature that for median run only one graph is shown 😉
I'm seeing on a screenshot that you've got InvalidProtoExpectation error. We already have reported issue for this - #39

@rpkoller thanks one more time for digging into this issue with us 👍
Can we close current one? Looks like it's solved.

@rpkoller
Copy link
Author

yep the initial issue seems to be fixed now 👍thx! and regarding InvalidProtoExpectation i noticed issue #39 already but thought it might be a different case, but then i noticed that it is still open so not fixed upstream ;)

and @denar90 one last question regarding your workflow with git npm i and npm link. when i am done with it and wanna delete that testing folder and revert back to my globally installed pwmetrics installed with npm i -g pwmetric is there anything i have to mind and apply or would it be enough to simply delete the folder containing the master version?

@denar90
Copy link
Collaborator

denar90 commented Feb 26, 2017

I suppose you can do smth like npm i -g pwmetrics it has to be enough to use latest version published on npm.

@denar90 denar90 closed this as completed Feb 26, 2017
@paulirish
Copy link
Owner

@denar90 since we now have typescript in the mix, we'll need to make sure the gsheets.js is in the published npm module. i bet it was excluded since npmignore inherits from gitignore.

@paulirish
Copy link
Owner

yeah. https://github.com/paulirish/pwmetrics/blob/master/.npmignore#L11 excludes that file. we should nuke that line and republish. ;)

@denar90
Copy link
Collaborator

denar90 commented Feb 27, 2017

should nuke that line and republish. ;)

agree, but don't publish until we solve stuff for #55.
Another thing, that current npm published version is far behind from current master...

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

No branches or pull requests

4 participants