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

Add global app property. #3549

Merged
merged 4 commits into from
Feb 8, 2023
Merged

Add global app property. #3549

merged 4 commits into from
Feb 8, 2023

Conversation

garg3133
Copy link
Member

@garg3133 garg3133 commented Jan 5, 2023

No description provided.

@garg3133 garg3133 requested a review from beatfactor January 5, 2023 14:45
@swrdfish
Copy link
Member

@garg3133 why do we need this?

@garg3133
Copy link
Member Author

garg3133 commented Jan 19, 2023

@swrdfish This was a suggestion made by Vishal to have an app variable globally just like the browser variable that users can use when writing tests for mobile apps.

@swrdfish
Copy link
Member

Sound good, also can you add a simple test?

@garg3133
Copy link
Member Author

I tried to look through the code, but couldn't find any tests written for any of the other global variables. So, maybe we can just have a example test written in examples/ folder, which is using app as a global variable?

lib/index.js Outdated
@@ -359,6 +359,20 @@ Object.defineProperty(Nightwatch, 'browser', {
}
});

Object.defineProperty(Nightwatch, 'app', {
Copy link
Member

Choose a reason for hiding this comment

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

Nit: we can use defineProperties, and define both app and browser

Copy link
Member Author

Choose a reason for hiding this comment

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

IIUC Object.defineProperties cannot be used to add the same descriptor to two properties, but is used to add all the properties in one go. But since all the properties are added to Nightwatch separately in this file, it makes more sense to me to use defineProperty only just to have a consistent pattern through the file.

But if the problem is with the repetition of the same descriptor for the two properties, we can maybe do something like this?

const nightwatchApiDescriptor = {
  configurable: true,
  get() {
    if (global.browser) {
      return global.browser;
    }

    const err = new TypeError('Nightwatch client is not yet available.');
    err.addDetailedErr = true;

    throw err;
  }
};

Object.defineProperty(Nightwatch, 'browser', nightwatchApiDescriptor);

Object.defineProperty(Nightwatch, 'app', nightwatchApiDescriptor);

Copy link
Member

Choose a reason for hiding this comment

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

Yes, can you do the refactoring to avoid repeating the code?

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

lib/testsuite/index.js Show resolved Hide resolved
@gravityvi
Copy link
Member

gravityvi commented Feb 1, 2023

should we add a basic high-level in apidemos test using global app property?

for example:

it('Search for Nightwatch', async function() {
  app  // available globally
    .click('id', 'org.wikipedia:id/search_container')
    .sendKeys('id', 'org.wikipedia:id/search_src_text', 'Nightwatch')
    .click({selector: 'org.wikipedia:id/page_list_item_title', locateStrategy: 'id', index: 0})
});

@beatfactor beatfactor merged commit 7e88978 into nightwatchjs:main Feb 8, 2023
harshit-bs pushed a commit to harshit-bs/nightwatch that referenced this pull request Mar 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants