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

Closes #138: Make SCENARIO_URLS dynamic and depending on the running test #141

Merged
merged 2 commits into from
Sep 19, 2024

Conversation

Miraeld
Copy link
Contributor

@Miraeld Miraeld commented Sep 17, 2024

Description

Fixes #138
This PR introduces a change that allows scenario URLs to be loaded conditionally based on the current test being run. This improves the efficiency of our tests by only loading the URLs that are needed for each test, which can lead to faster test execution and less flakiness on poor internet connections

Type of change

  • New feature (non-breaking change which adds functionality).
  • Bug fix (non-breaking change which fixes an issue).
  • Enhancement (non-breaking change which improves an existing functionality).
  • Breaking change (fix or feature that would cause existing functionality to not work as before).
  • Sub-task of #(issue number)
  • Release

Detailed scenario

To trigger the new code, simply run a test script. The scenario URLs for that specific test will be loaded from a JSON file and used in the test.

Technical description

Documentation

The code works by first importing a JSON file that maps script names to their respective scenario URLs. Then, in the wp.config.ts file, it retrieves the current script name using process.env.npm_lifecycle_event. This script name is used to look up the corresponding scenario URLs in the imported JSON object, and these URLs are assigned to SCENARIO_URLS.

New dependencies

No new dependencies were introduced in this change.

Risks

There are no known performance or security risks associated with this change. The main risk is that if the JSON file or the wp.config.ts file is not properly configured, the correct scenario URLs might not be loaded

Mandatory Checklist

Code validation

  • I validated all the Acceptance Criteria. If possible, provide screenshots or videos.
  • I triggered all changed lines of code at least once without new errors/warnings/notices.
  • I implemented built-in tests to cover the new/changed code.

Code style

  • I wrote a self-explanatory code about what it does.
  • I protected entry points against unexpected inputs.
  • I did not introduce unnecessary complexity.
  • Output messages (errors, notices, logs) are explicit enough for users to understand the issue and are actionnable.

Additional Checks

  • In the case of complex code, I wrote comments to explain it.
  • When possible, I prepared ways to observe the implemented system (logs, data, etc.).
  • I added error handling logic when using functions that could throw errors (HTTP/API request, filesystem, etc.)

@Miraeld Miraeld added the enhancement New feature or request label Sep 17, 2024
@Miraeld Miraeld requested review from jeawhanlee and a team September 17, 2024 13:23
@Miraeld Miraeld self-assigned this Sep 17, 2024
@Mai-Saad
Copy link
Contributor

Mai-Saad commented Sep 19, 2024

@Miraeld @jeawhanlee Thanks for the PR. on PR running test:smoke will run backstop. can you please check 🙏

@jeawhanlee
Copy link
Contributor

@Mai-Saad Do we have backstop references being created for urls with this PR, if not, I think we are good, what you mentioned will be fixed on a different issue here

@Mai-Saad
Copy link
Contributor

Do we have backstop references being created for urls with this PR

yes
screen-capture - 2024-09-19T134110.947.webm

@Miraeld
Copy link
Contributor Author

Miraeld commented Sep 19, 2024

@Mai-Saad Have you modified your own wp.config.ts to include the change related to SCENARIO_URLS.
Because I don't get any reference for smoke tests, so I'm wondering ...

You should have something like :

import ScenarioUrls from './scenarioUrls.json';

// ... Rest of your config

const scriptName = process.env.npm_lifecycle_event;
const SCENARIO_URLS = ScenarioUrls[scriptName];

// ... Rest of your config

export {
    WP_USERNAME,
    WP_PASSWORD,
    WP_BASE_URL,
    WP_ROOT_DIR,
    WP_ENV_TYPE,
    WP_DOCKER_CONTAINER,
    WP_DOCKER_ROOT_DIR,
    WP_SSH_USERNAME,
    WP_SSH_ADDRESS,
    WP_SSH_KEY,
    WP_SSH_ROOT_DIR,
    SCENARIO_URLS
};

@Mai-Saad
Copy link
Contributor

Mai-Saad commented Sep 19, 2024

@Miraeld you are right, I was using the old config. updated the config and no references with smoke now 🙏 and with llcssbg references are created

@Miraeld Miraeld requested a review from jeawhanlee September 19, 2024 11:12
@jeawhanlee
Copy link
Contributor

jeawhanlee commented Sep 19, 2024

I can confirm this as well.
Screenshot 2024-09-19 at 12 12 31

@hanna-meda
Copy link
Contributor

@Miraeld, @jeawhanlee, @Mai-Saad can confirm this, too.

Tested also with llimages, lcp, wpml for which no references will be created ✓
Screenshot 2024-09-19 at 15 10 56

Backstop references will be created only for llcssbg
Screenshot 2024-09-19 at 15 12 39

@Miraeld Miraeld merged commit a7e0390 into develop Sep 19, 2024
1 check passed
@Miraeld Miraeld deleted the enhancement/138-dynamic-scenario-urls branch September 19, 2024 13:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Create only necessary backstop reference during related test.
4 participants