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

Error in sonar analyzing this rule #610

Closed
gemal opened this issue Oct 28, 2017 · 12 comments · Fixed by #616
Closed

Error in sonar analyzing this rule #610

gemal opened this issue Oct 28, 2017 · 12 comments · Fixed by #616

Comments

@gemal
Copy link

gemal commented Oct 28, 2017

after the update to 0.13.0
https://sonarwhal.com/scanner/c094bc44-a740-4f8b-a813-de82bd9c90d0

@alrra alrra added the type:bug label Oct 28, 2017
@alrra
Copy link
Contributor

alrra commented Oct 28, 2017

@gemal Thanks for opening this issue!


I tested the URL using the CLI, and it works with all connectors.

@sarvaje Maybe this is because of a timeout (jsdom takes quite a bit to complete)?

@molant
Copy link
Member

molant commented Oct 30, 2017

@alrra the timeout shouldn't impact, we've had many sites with timeouts on axe.
We recently fixed a bug where a worker crash impacted the results of other crashes. It could be that we didn't fix it correctly or something else is going on. In any case, @sarvaje is investigating.

Thanks!

@sarvaje
Copy link
Contributor

sarvaje commented Oct 30, 2017

The line throwing the exception is this one.
The problem is that the manifest's content type is application/octet-stream. The requester is not setting the body.content.

Should we make something like this in the manifest-app-names rule?

const content = body.content || body.rawContent.toString()

or the content encoding has to be utf-8?

@molant
Copy link
Member

molant commented Oct 30, 2017

@alrra you are the one that knows more about the manifest spec. What do you think?

@molant
Copy link
Member

molant commented Oct 30, 2017

@gemal in the meantime if you want to see more results you can change your manifest encoding to utf-8 and serve it with the right mime type instead of application/octet-stream

@gemal
Copy link
Author

gemal commented Oct 30, 2017

according to https://developer.mozilla.org/en-US/docs/Web/Manifest:
Manifests should be served using the application/manifest+json MIME type. However, it is optional to do so.

@alrra
Copy link
Contributor

alrra commented Oct 31, 2017

or the content encoding has to be utf-8?

No.

The problem is that the manifest's content type is application/octet-stream.

In this type of cases, rules like this one should fail since the user basically serves binary data, so not valid JSON.

sarvaje added a commit to sarvaje/hint that referenced this issue Oct 31, 2017
Report an error if the manifest content type is not valid.

----------------------

Fix webhintio#610
@dylanb
Copy link

dylanb commented Nov 1, 2017

Is there anything we can do from the axe-core side to help with this problem?

@molant
Copy link
Member

molant commented Nov 1, 2017

Hi @dylanb,

For this specific issue the problem was in our code and #616 should fix it.

Nevertheless, the axe-core rule timeouts quite often with jsdom as you are already aware 😔

Do you know if there are any perf gains in axe-core's 3.x branch?
We could also try improving the VMs where we run sonar in the cloud see if that helps.
If you have any other ideas please let us know!

@dylanb
Copy link

dylanb commented Nov 1, 2017

@molant 3.0 does not contain any significant performance improvements although it does bypass the DOM for some operations so it might be faster in a JSDOM environment. you should try that out.

Out of pure interest, why do you use JSDOM rather than headless Chrome?

@molant
Copy link
Member

molant commented Nov 1, 2017

Out of pure interest, why do you use JSDOM rather than headless Chrome?

Lack of time to get all the things we want done 😔
The idea is to let the user chose in the future what connector they want to use in the online service so that will eventually arrive. For now we decided to go with jsdom because it was easier to set up in the environment and requires less resources to run so we could scale up easier if needed.

Opened an issue to track that. I'll let you know how it goes.

sarvaje added a commit to sarvaje/hint that referenced this issue Nov 1, 2017
Report an error if the manifest content type is not valid.

----------------------

Fix webhintio#610
sarvaje added a commit to sarvaje/hint that referenced this issue Nov 1, 2017
Report an error if the manifest content type is not valid.

----------------------

Fix webhintio#610
molant pushed a commit to sarvaje/hint that referenced this issue Nov 2, 2017
Report an error if the manifest content type is not valid.

----------------------

Fix webhintio#610
molant pushed a commit that referenced this issue Nov 2, 2017
Report an error if the manifest `content` is not text.

----------------------

Fix #610
@sarvaje
Copy link
Contributor

sarvaje commented Nov 3, 2017

@gemal You should be able to analyze your site without problems now:
https://sonarwhal.com/scanner/6f4035d9-9a6c-4583-a5d6-590cd119931f

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

Successfully merging a pull request may close this issue.

5 participants