added Puppeteer.executablePath tests #83
Conversation
|
Well now AppVeyor is working 😂 |
|
It's failing because: |
| }; | ||
| private string _downloadHost; | ||
|
|
||
| public const int DefaultRevision = 526987; |
There was a problem hiding this comment.
I can buy this now. But I created an issue to make it configurable in v1.0 #84
lib/PuppeteerSharp/Puppeteer.cs
Outdated
| { | ||
| var downloader = Downloader.CreateDefault(); | ||
| var revisionInfo = downloader.RevisionInfo(Downloader.CurrentPlatform, Downloader.DefaultRevision); | ||
| return revisionInfo.ExecutablePath; |
There was a problem hiding this comment.
This code is in the Launcher class on Puppeteer. Let's honor that.
https://github.com/GoogleChrome/puppeteer/blob/v1.0.0/lib/Launcher.js#L188
There was a problem hiding this comment.
should that method be static?
There was a problem hiding this comment.
Yes. Let's follow Puppetter API, it's static there.
lib/PuppeteerSharp/Launcher.cs
Outdated
| } | ||
|
|
||
| } | ||
| internal string GetExecutablePath() |
There was a problem hiding this comment.
Can we add one line between the bracket on line 138 and this new function?
…-executable-path-tests
|
|
||
| namespace PuppeteerSharp.Tests.Puppeteer | ||
| { | ||
| public class ExecutablePathTests |
There was a problem hiding this comment.
You need to make this class inherit from PuppeteerBaseTest and add the attribute [Collection("PuppeteerLoaderFixture collection")]
The problem we have here is that, unluckily, this is the first test running, so we need to download Chromium first, and PuppeteerBaseTest does that job for us.
There was a problem hiding this comment.
@Meir017 do you think you will be able to implement this by tomorrow?
If not I can merge it and implement it in another PR.
2 participants
closes #78