Get Nodejs to recognise jquery command for publishButton jest test file #764

NARUDESIGNS · 2021-12-13T14:04:52Z

Make sure these boxes are checked before your pull request (PR) is ready to be reviewed and merged. Thanks!

tests pass -- look for a green checkbox ✔️ a few minutes after opening your PR -- or run tests locally with grunt jasmine
code is in uniquely-named feature branch and has no merge conflicts
PR is descriptively titled
PR body includes fixes #0000-style reference to original issue #
ask @publiclab/reviewers for help, in a comment below

We're happy to help you get this ready -- don't be afraid to ask for help, and don't be discouraged if your tests fail at first!

If tests do fail, click on the red X to learn why by reading the logs.

Please be sure you've reviewed our contribution guidelines at https://publiclab.org/contributing-to-public-lab-software

We have a loose schedule of reviewing and pulling in changes every Tuesday and Friday, and publishing changes on Fridays.

Thanks!

gitpod-io · 2021-12-13T14:04:55Z

NARUDESIGNS · 2021-12-13T14:22:05Z

I noticed that node throws an error "$ is not defined" because jquery requires a window object. To fix this, I did:

   const {JSDOM} = require("jsdom");
   const myJSDom = new JSDOM(fs.readFileSync(path));
   $ = require('jquery')(myJSDom.window);

The first line requires jsdom which I think is already installed in the PublicLab.Editor project.

The second line in the snippet above reads the example/index.html file from the template you had wrote for all new jest test files:

path = fs.realpathSync('file://../examples/index.html');

The third line gets $ to point to the example/index.html file so we can do something like

// get the button text value
console.log($('.ple-publish').text());

Let me know your comments on how we can progress from here @jywarren

jywarren · 2021-12-13T18:29:27Z

This looks excellent, and if the test properly fails, that's a great sign. If you can fix the test and get it to turn green, then that's a full proof of concept and we could merge this!

I think the next steps would be to look at what we would need to add to each test file so that the rest of the Jasmine -> Jest test conversions will go smoothly. Ideally we can write this out in #743 so that we can make some of these into FTO issues for others to follow -- what do you think?

Or, you could make "empty" Jest test files for each Jasmine file, and the FTOs would be copying in and adjusting the Jasmine tests but not having to do the file creation. That way you could include these lines at the top of each file and newcomers wouldn't have to worry about them. What do you prefer?

NARUDESIGNS · 2021-12-13T23:07:20Z

I think the next steps would be to look at what we would need to add to each test file so that the rest of the Jasmine -> Jest test conversions will go smoothly. Ideally, we can write this out in #743 so that we can make some of these into FTO issues for others to follow -- what do you think?

I agree, I think this next step will be a lot easier after getting the tests to pass. But I got questions about the test case for the publish button. I'd reach out on the channel tomorrow morning.
@jywarren

jywarren · 2021-12-17T18:14:04Z

test/ui-testing/publishButton.test.js


+  test('something we are testing described here', () => {
+    // Check initially that Publish button is disabled.
+    expect($('.ple-publish').prop('disabled')).toBe(true);


OK this is what's failing now. And it looks like a real failure - but is it? In theory, the editor's Publish button should really be disabled on initial page load, right? because it's waiting for you to input a title and some text before it thinks you should be able to press Publish.

Maybe we should start by checking something more basic, like this?

expect($('.ple-publish')).toBeDefined();

That way we know we're looking at the correct page at least, then we can try to figure out why the button is not disabled. How does that sound?

I'm working from this list of Jest assertions: https://jestjs.io/docs/using-matchers

Ah, i see. From your gitter chat comment, you're saying the button gets created, but validation is never run (_editor.validate()), so it never adds the disabled state to the button because it never checks if you've added a title.

That would be good, in that we are reading the actual HTML. But why isn't it running validation? Normally these lines should "boot up" the editor including running initial validations:

PublicLab.Editor/examples/index.html

Lines 410 to 419 in f8e1edb

(function () {

editor = new PL.Editor({

textarea: $(".ple-textarea")[0],

destination: "/notes/create",

mapModule: true,

format: "publiclab",

lat: 23,

lon: 77,

zoom: 5,

});

And after all, the test file this one is based on does the exact same thing - and relies on that Editor "boot-up" to have run: https://github.com/publiclab/PublicLab.Editor/blob/main/spec/javascripts/button_spec.js

How can we see the errors that may occur preventing that bootup from happening? When we look through the log in this PR, are there signs that some code hit an error or did not run?

Earlier tests seem to have run. Did they load the page properly?https://github.com/publiclab/PublicLab.Editor/runs/4510360996?check_suite_focus=true#step:7:8

Hmm, a lot of them seem to be asynchronous, meaning they're waiting for other stuff to complete before trying to run. Do we need to do the same for this new test?

PublicLab.Editor/test/ui-testing/CustomInsert.test.js

Lines 9 to 13 in f8e1edb

test('Add Custom Insert text in rich text mode', async () => {

if (await page.evaluate(() => $(".woofmark-mode-markdown").is(":disabled"))) {

await page.click('.woofmark-mode-wysiwyg');

}

await page.waitForSelector('.ple-module-body');

So would that be like this, plus adding async to the top of the containing function?

Suggested change

expect($('.ple-publish').prop('disabled')).toBe(true);

await expect($('.ple-publish').prop('disabled')).toBe(true);

NARUDESIGNS · 2021-12-18T15:15:34Z

@jywarren I see the problem now.
Running scripts via jsdom is disabled by default. See jsdom repo
To run scripts, a second argument is passed to the jsdom constructor. This argument looks like this

{
    runScripts: "dangerously",
    resources: "usable"
}

When I add this, I notice it tries to run the scripts but then the google map script throws an error saying the browser (jsdom browser environment I think) is not supported

jywarren · 2021-12-19T14:47:42Z

Aha - well that's very good progress! So there are other ways to run a browser environment, and puppeteer actually uses full browsers run on the commandline, like chrome (sort of strangely called headless-chrome since it has no graphical user interface). And I think this is what Jest had been configured for - can we try switching from jsdom (which really more like simulates a browser JS environment) to a full "headless" browser?

jywarren · 2021-12-19T14:56:36Z

So i think by loading in jsdom, we are creating a secondary browser environment, and the default Jest is set up with is puppeteer already.

I was reading this page and it made some sense... https://github.com/smooth-code/jest-puppeteer/blob/master/packages/jest-environment-puppeteer/README.md

Or maybe https://stackoverflow.com/a/46987959/1116657 is a good guide.

This is starting to look more like what i had tried in #749 but honestly i got so turned around in that issue i bet it's not even very useful -- better to just start fresh.

jywarren · 2021-12-19T15:00:43Z

test/ui-testing/publishButton.test.js

-  test('something we are testing described here', () => {
+  var $;
+  beforeAll(async () => {
+    path = fs.realpathSync('file://../examples/index.html');


I'm not 100% sure here, but working from https://stackoverflow.com/a/46987959/1116657 I'm thinking we should try something like this:

Suggested change

path = fs.realpathSync('file://../examples/index.html');

import { launch } from 'puppeteer'

(async () => {

const browser = await launch({headless: false});

const page = await browser.newPage();

await page.goto('file://../examples/index.html', {waitUntil: 'networkidle'});

//path = fs.realpathSync('file://../examples/index.html');

await page.addScriptTag({url: 'https://code.jquery.com/jquery-3.2.1.min.js'});

await page.waitForNavigation()

My main uncertainty is how to load the local file rather than a remote URL. Not sure what I wrote here will work.

Yea exactly, I bumped into this as well since I decided I could no longer make use of jsdom. I tried this but it didn't work for me.
I think we might be able to load the local version of jquery using:

await page.addScriptTag({path: require.resolve('jquery')})

but I wasn't sure what that path (path to jquery) is for our project. So I did

const jqueryPath = require('jquery')

then I hovered on 'jquery' and saw the path, I copied it and used it directly. Here's the new error I got:

I'm not so sure what the '_productName' is but I'm still looking. Do you have any idea what it is?
@jywarren

jywarren · 2021-12-19T15:03:15Z

But then i wonder - can we just do exactly what's in this test, where we run stuff in the page using evaluate(), then use expect() to measure what we get back?

PublicLab.Editor/test/ui-testing/CustomInsert.test.js

Lines 62 to 64 in f8e1edb

    
           // Evaluate the expression 
        
           const stringIsIncluded2 = await page.evaluate(() => document.querySelector('.ple-textarea').value.includes('[notes:tag]')); 
        
           expect(stringIsIncluded2).toBe(true);

jywarren · 2021-12-19T22:43:51Z

The jQuery include should be ok like that but for reference the path is node_modules/jquery Hmm well it's saying there was an error in the asynchronous code but we didn't catch the error to report it. Let me make a suggestion to catch and display it.

…

On Sun, Dec 19, 2021, 2:07 PM Paul Ibeabuchi C. ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In test/ui-testing/publishButton.test.js <#764 (comment)> : > describe('Publish button', () => { - - test('something we are testing described here', () => { + var $; + beforeAll(async () => { + path = fs.realpathSync('file://../examples/index.html'); Yea exactly, I bumped into this as well since I decided I could no longer make use of jsdom. I tried this but it didn't work for me. I think we might be able to load the local version of jquery using: await page.addScriptTag({path: require.resolve('jquery')}) but I wasn't sure what that path (path to jquery) is for our project. So I did const jqueryPath = require('jquery') then I hovered on 'jquery' and saw the path, I copied it and used it directly. Here's the new error I got: [image: Screenshot 2021-12-19 at 19 17 08] <https://user-images.githubusercontent.com/49737323/146687480-b9302384-30fd-4456-b37c-402a35beabec.png> I'm not so sure what the '_productName' is but I'm still looking. Do you have any idea what it is? @jywarren <https://github.com/jywarren> — Reply to this email directly, view it on GitHub <#764 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAF6J7KQTBFAKMQPHJQWATURYUPBANCNFSM5J6HDHQQ> . Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>. You are receiving this because you were mentioned.Message ID: ***@***.***>

jywarren · 2021-12-19T22:54:21Z

Hmm I couldn't log in from a new phone but you have to add a second parameter to the async function to catch errors: test('something we are testing described here', async () => { // existing function code }, async (e) => { console.log(e) }) It's because promises take both a success and fail method, we just often skip the second. So the template is really: test(String name, Function success, Function fail) Does that make sense?

…

On Sun, Dec 19, 2021, 5:43 PM Jeffrey Yoo Warren ***@***.***> wrote: The jQuery include should be ok like that but for reference the path is node_modules/jquery Hmm well it's saying there was an error in the asynchronous code but we didn't catch the error to report it. Let me make a suggestion to catch and display it. On Sun, Dec 19, 2021, 2:07 PM Paul Ibeabuchi C. ***@***.***> wrote: > ***@***.**** commented on this pull request. > ------------------------------ > > In test/ui-testing/publishButton.test.js > <#764 (comment)> > : > > > > describe('Publish button', () => { > - > - test('something we are testing described here', () => { > + var $; > + beforeAll(async () => { > + path = fs.realpathSync('file://../examples/index.html'); > > Yea exactly, I bumped into this as well since I decided I could no longer > make use of jsdom. I tried this but it didn't work for me. > I think we might be able to load the local version of jquery using: > > await page.addScriptTag({path: require.resolve('jquery')}) > > but I wasn't sure what that path (path to jquery) is for our project. So > I did > > const jqueryPath = require('jquery') > > then I hovered on 'jquery' and saw the path, I copied it and used it > directly. Here's the new error I got: > > [image: Screenshot 2021-12-19 at 19 17 08] > <https://user-images.githubusercontent.com/49737323/146687480-b9302384-30fd-4456-b37c-402a35beabec.png> > > I'm not so sure what the '_productName' is but I'm still looking. Do you > have any idea what it is? > @jywarren <https://github.com/jywarren> > > — > Reply to this email directly, view it on GitHub > <#764 (comment)>, > or unsubscribe > <https://github.com/notifications/unsubscribe-auth/AAAF6J7KQTBFAKMQPHJQWATURYUPBANCNFSM5J6HDHQQ> > . > Triage notifications on the go with GitHub Mobile for iOS > <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> > or Android > <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>. > > You are receiving this because you were mentioned.Message ID: > ***@***.***> >

NARUDESIGNS · 2021-12-21T05:43:12Z

Does that make sense?

@jywarren yes it does. In fact, I had to wrap the code in a try...catch block so I can catch and log the errors but I got the same error. I could see where _productName was used in a puppeteer.js file but I'm not sure what it expects or what exactly it is. I have to look more into it.

jywarren · 2021-12-21T16:14:19Z

Ah ok - can you point me to the line in puppeteer? Whatever _____._productName is attached to is returning nil. Maybe we can search the Puppeteer issues for similar errors...

jywarren · 2021-12-21T16:15:37Z

Aha - maybe this will work: puppeteer/puppeteer#6266 (comment)

jywarren · 2021-12-21T19:52:29Z

I'm trying out the theory of "let's just use non-jQuery to reproduce this in Jest following the format in other Jest tests" idea in #767

Let's see how it does!

jywarren · 2022-01-04T19:35:02Z

OK, so let's close this up i think, just to keep things tidy? Thanks!

got nodejs to recognise jquery command for publishButton jest test file

ca17c06

Merge branch 'main' into fix-publish-button-jest-file

36ed578

jywarren reviewed Dec 17, 2021

View reviewed changes

jywarren mentioned this pull request Dec 19, 2021

[Planning] Refine the Public Lab Rich Editor #760

Open

31 tasks

jywarren reviewed Dec 19, 2021

View reviewed changes

jywarren closed this Jan 4, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Get Nodejs to recognise jquery command for publishButton jest test file #764

Get Nodejs to recognise jquery command for publishButton jest test file #764

NARUDESIGNS commented Dec 13, 2021

gitpod-io bot commented Dec 13, 2021

NARUDESIGNS commented Dec 13, 2021

jywarren commented Dec 13, 2021

NARUDESIGNS commented Dec 13, 2021 •

edited

Loading

jywarren Dec 17, 2021

jywarren Dec 17, 2021

jywarren Dec 17, 2021

jywarren Dec 17, 2021

NARUDESIGNS commented Dec 18, 2021 •

edited

Loading

jywarren commented Dec 19, 2021

jywarren commented Dec 19, 2021

jywarren Dec 19, 2021

NARUDESIGNS Dec 19, 2021

jywarren commented Dec 19, 2021

jywarren commented Dec 19, 2021 via email

jywarren commented Dec 19, 2021 via email

NARUDESIGNS commented Dec 21, 2021

jywarren commented Dec 21, 2021

jywarren commented Dec 21, 2021

jywarren commented Dec 21, 2021

jywarren commented Jan 4, 2022

	(function () {
	editor = new PL.Editor({
	textarea: $(".ple-textarea")[0],
	destination: "/notes/create",
	mapModule: true,
	format: "publiclab",
	lat: 23,
	lon: 77,
	zoom: 5,
	});

	test('Add Custom Insert text in rich text mode', async () => {
	if (await page.evaluate(() => $(".woofmark-mode-markdown").is(":disabled"))) {
	await page.click('.woofmark-mode-wysiwyg');
	}
	await page.waitForSelector('.ple-module-body');

	expect($('.ple-publish').prop('disabled')).toBe(true);
	await expect($('.ple-publish').prop('disabled')).toBe(true);

-    path = fs.realpathSync('file://../examples/index.html');
+import { launch } from 'puppeteer'
+(async () => {
+    const browser = await launch({headless: false});
+    const page = await browser.newPage();
+    await page.goto('file://../examples/index.html', {waitUntil: 'networkidle'});
+    //path = fs.realpathSync('file://../examples/index.html');
+    await page.addScriptTag({url: 'https://code.jquery.com/jquery-3.2.1.min.js'});
+    await page.waitForNavigation()

Get Nodejs to recognise jquery command for publishButton jest test file #764

Get Nodejs to recognise jquery command for publishButton jest test file #764

Conversation

NARUDESIGNS commented Dec 13, 2021

gitpod-io bot commented Dec 13, 2021

NARUDESIGNS commented Dec 13, 2021

jywarren commented Dec 13, 2021

NARUDESIGNS commented Dec 13, 2021 • edited Loading

jywarren Dec 17, 2021

Choose a reason for hiding this comment

jywarren Dec 17, 2021

Choose a reason for hiding this comment

jywarren Dec 17, 2021

Choose a reason for hiding this comment

jywarren Dec 17, 2021

Choose a reason for hiding this comment

NARUDESIGNS commented Dec 18, 2021 • edited Loading

jywarren commented Dec 19, 2021

jywarren commented Dec 19, 2021

jywarren Dec 19, 2021

Choose a reason for hiding this comment

NARUDESIGNS Dec 19, 2021

Choose a reason for hiding this comment

jywarren commented Dec 19, 2021

jywarren commented Dec 19, 2021 via email

jywarren commented Dec 19, 2021 via email

NARUDESIGNS commented Dec 21, 2021

jywarren commented Dec 21, 2021

jywarren commented Dec 21, 2021

jywarren commented Dec 21, 2021

jywarren commented Jan 4, 2022

NARUDESIGNS commented Dec 13, 2021 •

edited

Loading

NARUDESIGNS commented Dec 18, 2021 •

edited

Loading