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.

Sign up for GitHub

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

Enable nvda / firefox browser combination for automated testing #950

Merged
merged 2 commits into from
Mar 18, 2024
Merged

Enable nvda / firefox browser combination for automated testing #950

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a59e706
Enable nvda / firefox browser combination for automated testing
gnarf Mar 7, 2024
965481b
add support for other browsers to simulation and browser version crea…
gnarf Mar 13, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion client/utils/automation.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,10 @@ export const isSupportedByResponseCollector = ctx => {
if (!ctx || !ctx.at || !ctx.browser) {
return false;
}
return ctx.at.name === 'NVDA' && ctx.browser.name === 'Chrome';
return (
ctx.at.name === 'NVDA' &&
(ctx.browser.name === 'Chrome' || ctx.browser.name === 'Firefox')
);
};

export const isBot = user => user?.username?.toLowerCase().slice(-3) === 'bot';
Expand Down
20 changes: 9 additions & 11 deletions server/controllers/AutomationController.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -247,8 +247,12 @@ const updateJobResults = async (req, res) => {
testCsvRow,
presentationNumber,
responses,
atVersionName,
browserVersionName
capabilities: {
atName,
atVersion: atVersionName,
browserName,
browserVersion: browserVersionName
}
} = req.body;
const job = await getCollectionJobById({ id, transaction });
if (!job) {
Expand All @@ -261,15 +265,9 @@ const updateJobResults = async (req, res) => {
);
}

/* TODO: Change this once we support more At + Browser Combos in Automation */
const [at] = await getAts({
where: { name: 'NVDA' },
transaction
});
const [browser] = await getBrowsers({
where: { name: 'Chrome' },
transaction
});
/* TODO: Change this to use a better key based lookup system after gh-958 */
const [at] = await getAts({ search: atName, transaction });
const [browser] = await getBrowsers({ search: browserName, transaction });

const [atVersion, browserVersion] = await Promise.all([
findOrCreateAtVersion({
Expand Down
11 changes: 7 additions & 4 deletions server/middleware/transactionMiddleware.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,19 +23,17 @@ const middleware = async (req, res, next) => {
let transaction;
if (transactionId && transactions[transactionId]) {
transaction = transactions[transactionId];
} else if (transactionId) {
transaction = await sequelize.transaction();
transactions[transactionId] = transaction;
} else {
transaction = await sequelize.transaction();
transactions[transaction.id] = transaction;
}

req.transaction = transaction;

res.once('finish', async () => {
if (!isPersistentTransaction && req.transaction) {
await req.transaction.commit();
delete req.transaction;
delete transactions[transaction.id];
}
});

Expand Down Expand Up @@ -65,9 +63,14 @@ const forTestingRollBackTransaction = async transaction => {
delete transactions[transaction.id];
};

const getTransactionById = id => {
return transactions[id];
};

module.exports = {
middleware,
errorware,
getTransactionById,
forTestingPopulateTransaction,
forTestingRollBackTransaction
};
3 changes: 2 additions & 1 deletion server/models/services/CollectionJobService.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -360,7 +360,8 @@ const triggerWorkflow = async (job, testIds, { transaction }) => {
testPlanVersionGitSha: gitSha,
testIds,
testPlanName: directory,
jobId: job.id
jobId: job.id,
transactionId: transaction.id
},
axiosConfig
);
Expand Down
6 changes: 4 additions & 2 deletions server/services/GithubWorkflowService.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ const ALGORITHM = 'RS256';
// > the workflow file is on the default branch.
//
// https://docs.github.com/en/actions/using-workflows/events-that-trigger-workflows#workflow_dispatch
const WORKFLOW_FILE_NAME = 'nvda-chrome.yml';
const WORKFLOW_FILE_NAME = 'nvda-test.yml';
const WORKFLOW_REPO = 'bocoup/aria-at-gh-actions-helper';

// Generated from the GitHub.com UI
Expand Down Expand Up @@ -126,13 +126,15 @@ const createGithubWorkflow = async ({ job, directory, gitSha }) => {
jsonWebToken,
GITHUB_APP_INSTALLATION_ID
);
const browser = job.testPlanRun.testPlanReport.browser.name.toLowerCase();
Copy link
Contributor

Choose a reason for hiding this comment

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

There are keys provided for the browsers but it's not currently passed from the database or available in the resolvers.

It's definitely not a problem here but wanted to raise that, in case it may lighten and simplify the load on this service trying to derive a key from "VoiceOver for macOS".toLowerCase() for example, in the future. It would also maintain predictable key maps for the browsers across this app and the workflow(s) being triggered.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It would probably make more sense if this key you speak of if it is closer to being just "chrome" "firefox" "edge" etc. Browser currently seems to be defined:

    const Model = sequelize.define(
        MODEL_NAME,
        {
            id: {
                type: DataTypes.INTEGER,
                allowNull: false,
                primaryKey: true,
                autoIncrement: true
            },
            name: {
                type: DataTypes.TEXT,
                allowNull: false
            }
        },
        {
            timestamps: false,
            tableName: MODEL_NAME
        }
    );

Am I missing something here? It seems like browser is just id/name (and the name to lower also seem pretty safe in this case)

Copy link
Contributor

Choose a reason for hiding this comment

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

Oop! I realize now that I gave a bad and very confusing example because I must have viewed the AT table at the time I made the comment.

There are keys provided for the browsers but it's not currently passed from the database or available in the resolvers.

I was wrong on this. Still in my confusion, I was referencing ATs BUT those keys also aren't being tracked in the db, instead being derived. This comment is then irrelevant to this PR.

It seems like browser is just id/name (and the name to lower also seem pretty safe in this case)

It is safe!

But the point I'm making could still stand, in that there should be stored keys somehow. Not something for this PR but I think I'm just now becoming aware of a potential data consistency issue.

In the examples you gave here, when "Edge" gets added, they may want to present it as "Microsoft Edge" but the service (or other sections of the app) could want to resolve "Microsoft Edge" to "edge" for simplicity. Or if it ever comes to adding "Safari (for iOS)" -> "safari_ios", it would mean renaming the current "Safari" browser being tracked to something more appropriate like "Safari (for macOS)" which could have been implicitly being thought of as "safari_macos" anyways.

All that to say, I'd feel safer using:

job.testPlanRun.testPlanReport.browser.key;

VS

job.testPlanRun.testPlanReport.browser.name.toLowerCase();

OR

getBrowserKey(job.testPlanRun.testPlanReport.browser.name.toLowerCase());

If you or others agree to those keys, then that's a separate task to maybe make future needs easier.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

^ i don't feel entirely qualified to make this patch, the database stuff is still a bit of a mystery to me, but i do think that a "key" that isn't the "display name" here is probably the long-term "right" solution. (Followup issue?) I don't think we need to solve it to land this however.

Copy link
Contributor

@howard-e howard-e Mar 11, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

(Followup issue?) I don't think we need to solve it to land this however.

Agreed! I can make a follow up issue to discuss

Copy link
Contributor

Choose a reason for hiding this comment

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

Just adding a +1 to what @howard-e is suggesting!

Copy link
Contributor

Choose a reason for hiding this comment

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

#958

const inputs = {
callback_url: `https://${callbackUrlHostname}/api/jobs/${job.id}/result`,
status_url: `https://${callbackUrlHostname}/api/jobs/${job.id}/update`,
callback_header: `x-automation-secret:${process.env.AUTOMATION_SCHEDULER_SECRET}`,
work_dir: `tests/${directory}`,
test_pattern: '{reference/**,test-*-nvda.*}',
aria_at_ref: gitSha
aria_at_ref: gitSha,
browser
};
const axiosConfig = {
method: 'POST',
Expand Down
28 changes: 20 additions & 8 deletions server/tests/integration/automation-scheduler.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,14 +39,16 @@ const getTestPlanReport = async (id, { transaction }) =>
testPlanReport(id: "${id}") {
id
markedFinalAt
at { name }
browser { name }
finalizedTestResults {
test{
test {
id
}
atVersion{
atVersion {
name
}
browserVersion{
browserVersion {
name
}
scenarioResults {
Expand Down Expand Up @@ -265,7 +267,8 @@ describe('Automation controller', () => {
testPlanVersionGitSha,
testIds,
testPlanName,
jobId: parseInt(collectionJob.scheduleCollectionJob.id)
jobId: parseInt(collectionJob.scheduleCollectionJob.id),
transactionId: transaction.id
};

expect(axiosPostMock).toHaveBeenCalledWith(
Expand Down Expand Up @@ -491,8 +494,12 @@ describe('Automation controller', () => {
.post(`/api/jobs/${job.id}/result`)
.send({
testCsvRow: selectedTestRowNumber,
atVersionName: at.atVersions[0].name,
browserVersionName: browser.browserVersions[0].name,
capabilities: {
atName: at.name,
atVersion: at.atVersions[0].name,
browserName: browser.name,
browserVersion: browser.browserVersions[0].name
},
responses: new Array(numberOfScenarios).fill(
automatedTestResponse
)
Expand Down Expand Up @@ -556,6 +563,7 @@ describe('Automation controller', () => {
const selectedTestIndex = 0;
const selectedTestRowNumber = 1;

const { at, browser } = testPlanReport;
const historicalTestResult =
testPlanReport.finalizedTestResults[selectedTestIndex];
expect(historicalTestResult).not.toEqual(undefined);
Expand Down Expand Up @@ -584,8 +592,12 @@ describe('Automation controller', () => {
.post(`/api/jobs/${job.id}/result`)
.send({
testCsvRow: selectedTestRowNumber,
atVersionName: atVersion.name,
browserVersionName: browserVersion.name,
capabilities: {
atName: at.name,
atVersion: atVersion.name,
browserName: browser.name,
browserVersion: browserVersion.name
},
responses: historicalResponses
})
.set(
Expand Down
95 changes: 50 additions & 45 deletions server/tests/util/mock-automation-scheduler-server.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,8 +6,11 @@ const {
const { COLLECTION_JOB_STATUS } = require('../../util/enums');
const { default: axios } = require('axios');
const { gql } = require('apollo-server-core');
const apolloServer = require('../../graphql-server');
const { axiosConfig } = require('../../controllers/AutomationController');
const {
getTransactionById
} = require('../../middleware/transactionMiddleware');
const { query } = require('../util/graphql-test-utilities');

const setupMockAutomationSchedulerServer = async () => {
const app = express();
Expand Down Expand Up @@ -35,7 +38,9 @@ const setupMockAutomationSchedulerServer = async () => {

const simulateResultCompletion = async (
tests,
atName,
atVersionName,
browserName,
browserVersionName,
jobId,
currentTestIndex,
Expand All @@ -52,8 +57,12 @@ const setupMockAutomationSchedulerServer = async () => {
});

const testResult = {
atVersionName,
browserVersionName,
capabilities: {
atName,
atVersion: atVersionName,
browserName,
browserVersion: browserVersionName
},
responses
};

Expand All @@ -74,7 +83,9 @@ const setupMockAutomationSchedulerServer = async () => {
setTimeout(() => {
simulateResultCompletion(
tests,
atName,
atVersionName,
browserName,
browserVersionName,
jobId,
currentTestIndex + 1,
Expand Down Expand Up @@ -102,64 +113,56 @@ const setupMockAutomationSchedulerServer = async () => {
});
} else {
// Local development must simulate posting results
const { testPlanVersionGitSha, testPlanName, jobId } = req.body;

const {
data: { testPlanVersions }
} = await apolloServer.executeOperation({
query: gql`
const { jobId, transactionId } = req.body;
const transaction = getTransactionById(transactionId);
const data = await query(
gql`
query {
testPlanVersions {
id
gitSha
metadata
testPlan {
id
}
testPlanReports {
at {
name
atVersions {
name
collectionJob(id: "${jobId}") {
testPlanRun {
testPlanReport {
testPlanVersion {
metadata
gitSha
}
}
browser {
name
browserVersions {
at {
name
atVersions {
name
}
}
}
runnableTests {
id
rowNumber
scenarios {
id
browser {
name
browserVersions {
name
}
}
assertions {
runnableTests {
id
rowNumber
scenarios {
id
}
assertions {
id
}
}
}
}
}
}
`
});

const testPlanVersion = testPlanVersions.find(
testPlanVersion =>
testPlanVersion.gitSha === testPlanVersionGitSha &&
testPlanVersion.testPlan.id === testPlanName
);

const testPlanReport = testPlanVersion.testPlanReports.find(
testPlanReport =>
testPlanReport.at.name === 'NVDA' &&
testPlanReport.browser.name === 'Chrome'
`,
{ transaction }
);
const { collectionJob } = data;
const { testPlanReport } = collectionJob.testPlanRun;
const { testPlanVersion } = testPlanReport;

const browserName = testPlanReport.browser.name;
const browserVersionName =
testPlanReport.browser.browserVersions[0].name;

const atName = testPlanReport.at.name;
const atVersionName = testPlanReport.at.atVersions[0].name;
const { runnableTests } = testPlanReport;

Expand All @@ -178,7 +181,9 @@ const setupMockAutomationSchedulerServer = async () => {
() =>
simulateResultCompletion(
runnableTests,
atName,
atVersionName,
browserName,
browserVersionName,
jobId,
0,
Expand Down
Loading