Implement Console Events and Page.close tests #134

PhenX · 2018-04-12T17:50:16Z

closes #13
closes #5

Meir017 · 2018-04-12T18:04:10Z

lib/PuppeteerSharp/ConsoleMessage.cs

+
+        public string Text()
+        {
+            return string.Join(" ", Args);


use arrow function

Meir017 · 2018-04-12T18:08:46Z

lib/PuppeteerSharp/Messaging/PageConsoleResponse.cs

+    public class PageConsoleResponse
+    {
+        [JsonProperty("type")]
+        public string Type { get; set; }


Newtonsoft.Json knows how to deserialize enums (StringEnumConverter) so we can use ConsoleType here and remove the conversion in the
ConsoleMessage class

@Meir017 I got "inspiration" from Dialog.cs made by @kblok , which uses a mapping method but you are right. @kblok what do you think ?

If you can also improve Dialog go for it :)

I didn't think of it back then...
I think I thought that it wouldn't work when there are multiple words (DialogType.BeforeUnload), I just tested it and it works.

[JsonConverter(typeof(StringEnumConverter), true)] public enum DialogType { Alert, Prompt, Confirm, BeforeUnload }; JsonConvert.DeserializeObject<DialogType>("beforeunload"); // output: BeforeUnload

It only works when deserializing since:

JsonConvert.SerializeObject(DialogType.BeforeUnload) // output: "\"beforeUnload\""

and not beforeunload

A possible solution is to use [EnumMember(Value = "beforeunload")] for enum values where the default cameCase doesn't work, does this make sense?

I made the changes, but didn't include "[JsonConverter(typeof(StringEnumConverter), true)]", do you confirm it's necessary ? It works find without it to deserialize, do we need to serialize these values too ?

If we don't really need it, don't add it.

PhenX · 2018-04-13T07:22:12Z

I think everything is OK now, do you confirm?

Meir017 · 2018-04-13T07:24:07Z

lib/PuppeteerSharp/Page.cs

-        private void OnConsoleAPI(MessageEventArgs e)
+        private async Task OnConsoleAPI(PageConsoleResponse message)
        {
+            // TODO Do we need to translate this part ?


do we need these comments? ;)

I think I have my answer :)

Meir017 · 2018-04-13T07:25:26Z

lib/PuppeteerSharp/Page.cs

+            }
+
+            var handles = message.Args
+                .Select(_ => (JSHandle)_frameManager.CreateJsHandle(message.ExecutionContextId, _))


do we need to cast here?

Yes, I don't really understand why, but it doesn't recognize the return type of CreateJsHandle :

Unable to cast object of type 'System.Collections.Generic.List<dynamic>' to type 'System.Collections.Generic.IList<PuppeteerSharp.JSHandle>'

it's because message.Args is dynamic

Yes, that's normal, it's the original data, not yet transformed into a JSHandle

too bad.
ok

First time I see _ in use, fancy!

What would be the benefit of using _ instead of defaulting that argument to null in the method?

Meir017 · 2018-04-13T07:39:58Z

lib/PuppeteerSharp/ConsoleType.cs

@@ -0,0 +1,14 @@
+namespace PuppeteerSharp
+{
+    public enum ConsoleType


I this should have more options - https://github.com/DefinitelyTyped/DefinitelyTyped/blob/master/types/puppeteer/index.d.ts#L133

Fixed, thanks

…res/page.events.console

…to features/page.events.console

PhenX · 2018-04-13T10:21:35Z

Argh, I think I did a mistake :/

Meir017 · 2018-04-13T11:20:56Z

if it's too hard to revert you can edit the description of this pull-request and add another closes #xx line
@kblok would this be okey since this is the last task?

kblok · 2018-04-13T11:28:27Z

Yeah I'm ok with that @PhenX @Meir017, change the description so the project can track it

PhenX · 2018-04-13T11:32:02Z

@Meir017 @kblok thanks I was starting to panic :)

kblok · 2018-04-13T11:37:01Z

lib/PuppeteerSharp.Tests/Page/Events/ConsoleTests.cs

+            Assert.Equal("5", (await message.Args[1].JsonValue()).ToString());
+
+            // TODO Json Serialization is not exactly the same as in Puppeteer
+            Assert.Equal("{\r\n  \"foo\": \"bar\"\r\n}", (await message.Args[2].JsonValue()).ToString());


Here we should get a json, object, and in line 30 we should get a 5 as an int.

I think that ValueFromRemoteObject should try to parse that as a JSON value. I should succeed parsing strings, numbers, and objects.

You are right, I'll change it to use remoteObject.type (https://chromedevtools.github.io/devtools-protocol/tot/Runtime#type-RemoteObject) with a switch to handle all the cases, what do you think ?

Yeah, sounds good

maybe make JsonValue return Task<T> instead of Task<object>?

Thinking about this a little bit, the doc says:

Remote object value in case of primitive values or JSON values (if it was requested).

So if we fall there it's because it's serializable, you can just use JSON.Parse in one line

Yes, we can make it generic, but let's leave that conversion to JSON.NET.

I didn't find a way to make it work for the third assert (with {foo: "bar"}) without using generics, and for the second (with "5"), there are no integers in JS, only numbers, so I have to force it to float. You'll see all this in my next commit.

kblok · 2018-04-13T11:38:35Z

lib/PuppeteerSharp/FrameManager.cs

-            });
+        private void OnExecutionContextCreated(ContextPayload contextPayload)
+        {
+


Let's remove this empty line

kblok · 2018-04-13T11:41:58Z

lib/PuppeteerSharp/Page.cs

+            }
+
+            var handles = message.Args
+                .Select(_ => (JSHandle)_frameManager.CreateJsHandle(message.ExecutionContextId, _))


First time I see _ in use, fancy!

What would be the benefit of using _ instead of defaulting that argument to null in the method?

PhenX · 2018-04-13T13:50:44Z

The "_" is not null, it corresponds to the items from the Args array. I use underscore for Queryables and Enumerables by habit :)

kblok · 2018-04-13T14:00:13Z

How about the one on CreateJsHandle @PhenX? #LearningMode

PhenX · 2018-04-13T14:07:54Z

Hum, I'm not sure to understand, do we agree that this :

    var handles = message.Args
        .Select(_ => (JSHandle)_frameManager.CreateJsHandle(message.ExecutionContextId, _))

is the same as this ?

    var handles = message.Args
        .Select(item => (JSHandle)_frameManager.CreateJsHandle(message.ExecutionContextId, item))

or even this ?

    var handles = new List<JSHandle>();
    foreach (var arg in message.Args)
    {
        handles.Add(_frameManager.CreateJsHandle(message.ExecutionContextId, arg));
    }

Meir017 · 2018-04-13T14:10:01Z

I think that once one of the argument is of type dynamic then all of the expression is dynamic but I'm not sure.
The casting will be removed once everything is strongly typed

PhenX · 2018-04-13T14:13:42Z

@Meir017 yes, I think that's exactly this, I see it with the remoteObject object, I need to convert it explicitly to string in a string variable to get something other than dynamic.

…ow real type instead of dynamic value

kblok · 2018-04-17T12:09:31Z

How are we going with this guy @PhenX ?

PhenX · 2018-04-18T20:02:49Z

What do you think about my latest changes on jsonvalue?

kblok · 2018-04-18T20:04:27Z

lib/PuppeteerSharp.Tests/Page/Events/ConsoleTests.cs

-            Assert.Equal("{\r\n  \"foo\": \"bar\"\r\n}", (await message.Args[2].JsonValue()).ToString());
+            Assert.Equal("hello", await message.Args[0].JsonValue());
+            Assert.Equal(5, await message.Args[1].JsonValue<float>());
+            Assert.Equal(obj,await message.Args[2].JsonValue<Dictionary<string, object>>());


How about testing this?

Assert.Equal("bar", await message.Args[2].JsonValue<dynamic>().foo);

Yes it could be in addition to the current test

kblok · 2018-04-18T20:06:12Z

lib/PuppeteerSharp/Helper.cs

+            switch (objectType)
+            {
+                case "object":
+                    return JsonConvert.DeserializeObject<T>(objectValue);


I think JsonConvert will be able to serialize a "number" a "boolean" and a "bigint", what do you think?

Yes it's pretty sure it can

kblok · 2018-04-18T20:06:40Z

Looking good!
Could you try pulling from master and see if it builds?

PhenX · 2018-04-18T20:11:15Z

I'll try and do that tomorrow. When do you think you'll release 0.4?

kblok · 2018-04-18T20:11:54Z

I want to release it during the weekend, we are almost there

kblok · 2018-04-18T20:21:36Z

lib/PuppeteerSharp.Tests/Page/CloseTests.cs

+            var newPage = await Browser.NewPageAsync();
+            var neverResolves = newPage.EvaluateFunctionAsync("() => new Promise(r => {})");
+
+            await newPage.CloseAsync();


One thing I noticed on Puppeteer is that they are not awaiting the close
https://github.com/GoogleChrome/puppeteer/blob/v1.0.0/test/test.js#L325

kblok · 2018-04-18T20:24:31Z

lib/PuppeteerSharp/ConsoleMessage.cs

+        public ConsoleType Type { get; }
+        public IList<JSHandle> Args { get; }
+
+        public ConsoleMessage(ConsoleType type, IList<JSHandle> args)


ConsoleMessage has a text property which is different from the args

https://github.com/GoogleChrome/puppeteer/blob/v1.0.0/lib/Page.js#L447

Yes, at the beginning it was present, I didn't see the purpose ... I replaced it by this in ConsoleMessage which does the same

public string Text() => string.Join(" ", Args);

If really needed I'll change it

Yup, we have to honor Puppeteer

…e.events.console

…res/page.events.console

PhenX · 2018-04-19T15:42:55Z

(•_•)
( •_•)>⌐■-■
(⌐■_■)

kblok · 2018-04-19T15:48:21Z

Looks good to me, what do you think @Meir017 ?

kblok · 2018-04-19T16:06:19Z

@PhenX you have officially completed version 0.4 :)

PhenX · 2018-04-19T16:12:31Z

:D

Implement Console Events, including unit tests

4d585b9

PhenX changed the title ~~Implement Console Events, including unit tests~~ Implement Console Events Apr 12, 2018

Meir017 reviewed Apr 12, 2018

View reviewed changes

lib/PuppeteerSharp/ConsoleMessage.cs Outdated

public string Text()

{

return string.Join(" ", Args);

Copy link

Member

Meir017 Apr 12, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

use arrow function

Use arrow function

b09dd8e

Meir017 reviewed Apr 12, 2018

View reviewed changes

Use native deserialization mapping for ConsoleType and DialogType

022a361

Meir017 reviewed Apr 13, 2018

View reviewed changes

PhenX added 5 commits April 13, 2018 09:40

Remove uncessary comments

de650e2

Add missing console types

1622e8e

Merge remote-tracking branch 'remotes/kblok/origin/master' into featu…

d2d1e07

…res/page.events.console

Implement Page.close tests

5aab902

Merge remote-tracking branch 'origin/features/page.events.console' in…

c514d7c

…to features/page.events.console

PhenX changed the title ~~Implement Console Events~~ Implement Console Events and Page.close tests Apr 13, 2018

kblok requested changes Apr 13, 2018

View reviewed changes

Remove empty line

93e950e

Use generics for JsonValue and ValueFromRemoteObject, which returns n…

537234a

…ow real type instead of dynamic value

kblok reviewed Apr 18, 2018

View reviewed changes

kblok requested changes Apr 18, 2018

View reviewed changes

PhenX and others added 3 commits April 19, 2018 09:16

Merge remote-tracking branch 'remotes/kblok/master' into features/pag…

a927288

…e.events.console

Various fixes

ba7e3ce

Merge remote-tracking branch 'remotes/kblok/origin/master' into featu…

70040b6

…res/page.events.console

kblok approved these changes Apr 19, 2018

View reviewed changes

Meir017 approved these changes Apr 19, 2018

View reviewed changes

kblok merged commit 5858143 into hardkoded:master Apr 19, 2018

Uh oh!

Implement Console Events and Page.close tests #134

Implement Console Events and Page.close tests #134

Uh oh!

Conversation

PhenX commented Apr 12, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

PhenX commented Apr 13, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

PhenX commented Apr 13, 2018

Uh oh!

Meir017 commented Apr 13, 2018

Uh oh!

kblok commented Apr 13, 2018

Uh oh!

PhenX commented Apr 13, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

PhenX commented Apr 13, 2018

Uh oh!

kblok commented Apr 13, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

PhenX commented Apr 13, 2018

PhenX commented Apr 12, 2018 •

edited

Loading

kblok commented Apr 13, 2018 •

edited

Loading