Rearranged typeof check to handle null parameter #2494

sjana-aj · 2020-07-17T20:27:16Z

Error:
Cannot read property 'toString' of null
typeError: Cannot read property 'toString' of null
at G:\CodeceptJS-Test\node_modules\codeceptjs\lib\step.js:115:22

Solution: Rearranged typeof check to handle crash when helper method is called with null parameter.

Motivation/Description of the PR

Resolves Invoking custom helper I.methodName(null) method resulted into crash due to a logic error in lib/step.js #2493 .

Applicable helpers:

Type of change

Checklist:

Tests have been added
Documentation has been added (Run npm run docs)
Lint checking (Run npm run lint)
Local tests are passed (Run npm test)

Error: Cannot read property 'toString' of null typeError: Cannot read property 'toString' of null at G:\CodeceptJS-Test\node_modules\codeceptjs\lib\step.js:115:22 Solution: Rearranged typeof check to handle crash when helper method is called with null parameter.

Vorobeyko · 2020-07-18T09:53:38Z

lib/step.js

      } else if (typeof arg === 'object') {
        return JSON.stringify(arg);
+      } else if (arg.toString && arg.toString() !== '[object Object]') {


You can checks that arg is not null (undefined) at the beginning.
arg && arg.toString && arg.toString() !== '[object Object]'
And not rearrange conditions.

@Vorobeyko pls see my comment below

Vorobeyko · 2020-07-18T09:57:17Z

lib/step.js

@@ -112,10 +112,10 @@ class Step {
        return `${arg}`;
      } else if (arg instanceof Secret) {
        return '*****';
-      } else if (arg.toString && arg.toString() !== '[object Object]') {
-        return arg.toString();
      } else if (typeof arg === 'object') {


And if arg is null, this line will return true, because null it is object )
And humanizeArgs return sting like "null".

Maybe we checks of null as first condition?

@Vorobeyko else if (typeof arg === 'object') handles when arg is sent as null and humanizeArgs return sting "null" but the real issue here is that else if (arg.toString && arg.toString() !== '[object Object]') check comes before else if (typeof arg === 'object') check and breaks the condition as arg.toString() fails to work on arg=null value, so I thought of not adding any extra check and rearrange our checks a bit which will not hopefully create any side effect and handles the null condition well. Would love to hear your feedback on this pls.

@Vorobeyko just wanted to check if you found time to look through my comment pls

@sjana-aj Hi, sorry for late.
I think you must to add additionally checks on top like:

if (!args) { return '' }

And this all.
Args never been null or undefined on line arg.toString && arg.toString() !== '[object Object]'

DavertMik · 2020-07-28T18:52:50Z

Sorry, @sjana-aj
@Vorobeyko might be on vacation and I think I will need his approval to merge this PR
I hope we will do that soon

sjana-aj · 2020-07-29T10:54:37Z

np at all @DavertMik , thanks for letting me know 👍

Vorobeyko · 2020-08-03T05:02:24Z

lib/step.js

@@ -112,10 +112,10 @@ class Step {
        return `${arg}`;
      } else if (arg instanceof Secret) {
        return '*****';
-      } else if (arg.toString && arg.toString() !== '[object Object]') {
-        return arg.toString();
      } else if (typeof arg === 'object') {


@sjana-aj Hi, sorry for late.
I think you must to add additionally checks on top like:

if (!args) { return '' }

And this all.
Args never been null or undefined on line arg.toString && arg.toString() !== '[object Object]'

sjana-aj · 2020-08-03T06:05:30Z

@Vorobeyko thanks for your comments. I just made the changes, can you pls check again?

Vorobeyko · 2020-08-03T06:34:32Z

lib/step.js

@@ -95,6 +95,9 @@ class Step {

  /** @return {string} */
  humanizeArgs() {
+    if ( !args ) {


no no. You must to move this to this.args.map((arg) => {
And use only arg

I see, updated method now, pls check again

hm... interested, why some CI jobs not started)

yup, I noticed that too. Is there some log available to investigate the cause?

DavertMik · 2020-08-04T20:32:48Z

Thank you!

sjana-aj · 2020-08-04T21:36:55Z

Thank you @DavertMik 👍

* updated docs * release 2.6.5 * Fix valid data output (w3) (#2399) * add alt for image UI page (fix w3 validator) (#2403) * docs: fix typo in commands.md (#2405) * remove unnecessary select placeholder from basics.md (#2407) * fix display combination in changelog (#2409) * add description to images (#2412) * Fix android native locator support for appium helper (#2429) * Update mobile.md * Fix android native locator support for appium helper * update the expected result for windows platform especially the absolute path case because in windows, path.resolve returns driver letter * chore: jsdoc color param optional in `I.say` (#2443) * update github links after repository migration (#2447) * Fixed waitNumberOfVisibleElements for Webdriver (#2418) * fix waitNumberOfVisibleElements waitNumberOfVisibleElements returns false everytime as it does not filter the visible element count * fix waitNumberOfVisibleElements waitNumberOfVisibleElements returns false everytime as it does not filter for visible elements * Issue 2434 (handle .indexOf error) (#2436) * Makes suggested changes to have Chromium install * fixes #2434 * fix playwright too Co-authored-by: George Griffiths <george.griffiths@ibm.com> * Puppeteer handle page error (#2435) * Makes suggested changes to have Chromium install * handle page crash * handle undefined page * dont open new tab on crash * remove old try catch Co-authored-by: George Griffiths <george.griffiths@ibm.com> * Feat: Skip feature (#2427) * Makes suggested changes to have Chromium install * feat: implement skip of features * Add unit tests, docs and type defs * Apply suggestions from code review Co-authored-by: George Griffiths <george.griffiths@ibm.com> Co-authored-by: Michael Bodnarchuk <DavertMik@users.noreply.github.com> * Release 2.6.6 (#2450) * updated docs * fixed tests & updated linter * updated changelog * fixed mocks * Honor reporter configuration in mocha (#2465) * Update compilerOptions.target to es2017 (#2484) This resolves (#2483), since async and await are no long transpiled to ES generators * Fix playwright set cookie (#2491) * Fix setCookie for Playwright helper * fix lint issue Co-authored-by: Ben Barker <benbarker@workfront.com> * Add documentation changes for check-tests (#2502) * fix markdown video links (#2489) * Update pageobjects.md (#2497) * update plugins page "h" tags for more readable (#2501) * Fix typo in locators docs section (#2503) * Right click issue fix for Webdriver (#2485) * Right click issue fix for Webdriver * moveTo added in rightclick * Native Click used to perform rightclick operation * Added `forceRightClick` method to emulate rightclick event instead of using native events. * Add --invert option for run-workers command (#2504) * add missing REST helper (#2474) * Release 2.6.7 (#2506) * Prepare release-2.6.7 * Apply ESLint fixes. * updated playwright workflow to use official action Co-authored-by: Paul Vincent Beigang <paul.beigang@digistore24.com> Co-authored-by: Davert <davert.ua@gmail.com> * slight updates to changelog * added forceRightClick method * Fix missing screenshots on failure when REST helper is in use (#2513) * Improve error logging (#2512) * getPageTimeout: 30000, (#2516) fix(page-timeouts): method amOnPage freezing, when equal 0. * update to relevant path & some fix (#2515) * Update init.js (#2520) * element screenshot method added (#2521) * Rearranged typeof check to handle null parameter (#2494) * Rearranged typeof check to handle null parameter Error: Cannot read property 'toString' of null typeError: Cannot read property 'toString' of null at G:\CodeceptJS-Test\node_modules\codeceptjs\lib\step.js:115:22 Solution: Rearranged typeof check to handle crash when helper method is called with null parameter. * Update step.js * Update step.js * Prepare 2.6.8 release. (#2514) * Prepare 2.6.8 release. * Prepare 2.6.8 release. Co-authored-by: Paul Vincent Beigang <paul.beigang@digistore24.com> * Fix generate:helper command (#2523) * Release 2.6.8 (#2522) * added docs * added type method * fixed steps test * fixed tests & types * fixed tests & types * fixed tests for type * added type to Protractor * fixed typing via Protractor * removed protractor type * removed protractor type * fixed tests * fixed tests * fixed tests * updated release paths * feat(helper): Clear SessinStorage (#2524) * Fix broken URLs (#2528) * update readme file urls (#2534) * update docs md to relevant path (#2530) * Preserve initial error stack when helper load fail (#2541) * Release 2.6.9 (#2545) * fixed tests Co-authored-by: Davert <davert.ua@gmail.com> Co-authored-by: Mykhailo Bodnarchuk <mykhailo.bodnarchuk@Mykhailos-MacBook-Pro.local> Co-authored-by: Ihor Sychevskyi <26163841+Arhell@users.noreply.github.com> Co-authored-by: Aleksei Gurianov <gurianov@gmail.com> Co-authored-by: Tanakiat Srisaranyakul <tanakiats@hotmail.com> Co-authored-by: Bartosz Wojtkowiak <bartosz@wojtkowiak.it> Co-authored-by: Vijay Venkatesh <ilangovan.vijay@gmail.com> Co-authored-by: George Griffiths <georgegriffiths@live.com> Co-authored-by: George Griffiths <george.griffiths@ibm.com> Co-authored-by: Michael Bodnarchuk <DavertMik@users.noreply.github.com> Co-authored-by: Trinh Pham <9128061+trinhpham@users.noreply.github.com> Co-authored-by: Shan <shanplourde@gmail.com> Co-authored-by: Ben Barker <barker.ben.m@gmail.com> Co-authored-by: Ben Barker <benbarker@workfront.com> Co-authored-by: Koushik Mohan <koushikmohan1996@gmail.com> Co-authored-by: Jesus Vilar <jesus.vilar@gmail.com> Co-authored-by: Marcin Słowiak <marcin.slowiak.007@gmail.com> Co-authored-by: suniljaiswal01 <45997477+suniljaiswal01@users.noreply.github.com> Co-authored-by: Paul <pbeigang@gmail.com> Co-authored-by: Peter Nguyen Tr <peter.nguyentr@gmail.com> Co-authored-by: Paul Vincent Beigang <paul.beigang@digistore24.com> Co-authored-by: Sitam Jana <43989292+sjana-aj@users.noreply.github.com> Co-authored-by: Leonardo Bazico <leonardobazico@gmail.com> Co-authored-by: Igor Strebezhev <xamgore@users.noreply.github.com> Co-authored-by: Michail Shipov <mshipov@yandex.ru>

Vorobeyko requested changes Jul 18, 2020

View reviewed changes

DavertMik previously approved these changes Jul 27, 2020

View reviewed changes

Vorobeyko requested changes Aug 3, 2020

View reviewed changes

Update step.js

5323de1

sjana-aj dismissed DavertMik’s stale review via 5323de1 August 3, 2020 06:03

Vorobeyko approved these changes Aug 3, 2020

View reviewed changes

Vorobeyko requested changes Aug 3, 2020

View reviewed changes

Update step.js

dc88d9e

DavertMik requested a review from Vorobeyko August 3, 2020 21:52

DavertMik approved these changes Aug 3, 2020

View reviewed changes

DavertMik merged commit fa84f3f into codeceptjs:master Aug 4, 2020

DavertMik approved these changes Aug 4, 2020

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rearranged typeof check to handle null parameter #2494

Rearranged typeof check to handle null parameter #2494

sjana-aj commented Jul 17, 2020 •

edited

Vorobeyko Jul 18, 2020

sjana-aj Jul 20, 2020

Vorobeyko Jul 18, 2020

sjana-aj Jul 20, 2020 •

edited

sjana-aj Jul 23, 2020

Vorobeyko Aug 3, 2020

DavertMik commented Jul 28, 2020

sjana-aj commented Jul 29, 2020

Vorobeyko Aug 3, 2020

sjana-aj commented Aug 3, 2020

Vorobeyko Aug 3, 2020 •

edited

sjana-aj Aug 3, 2020

Vorobeyko Aug 3, 2020

sjana-aj Aug 3, 2020

DavertMik commented Aug 4, 2020

sjana-aj commented Aug 4, 2020

Rearranged typeof check to handle null parameter #2494

Rearranged typeof check to handle null parameter #2494

Conversation

sjana-aj commented Jul 17, 2020 • edited

Motivation/Description of the PR

Type of change

Checklist:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sjana-aj Jul 20, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DavertMik commented Jul 28, 2020

sjana-aj commented Jul 29, 2020

Choose a reason for hiding this comment

sjana-aj commented Aug 3, 2020

Vorobeyko Aug 3, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DavertMik commented Aug 4, 2020

sjana-aj commented Aug 4, 2020

sjana-aj commented Jul 17, 2020 •

edited

sjana-aj Jul 20, 2020 •

edited

Vorobeyko Aug 3, 2020 •

edited