Enable nvda / firefox browser combination for automated testing

[gnarf]

Tested locally seems fine!

[howard-e]

Looks good! Did a test locally (to ensure the chrome build hasn't changed)

Tiny nitpick - I haven't reviewed this file closely before but there's a typo on L42: 'Envrionment ... if you'd also like to include that here

[howard-e]

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.

[gnarf]

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)

[howard-e]

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.

[gnarf]

^ 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.

[howard-e]

(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

[stalgiag]

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

[howard-e]

#958

[gnarf]

Tiny nitpick - I haven't reviewed this file closely before but there's a typo on L42: 'Envrionment ... if you'd also like to include that here

#954

[stalgiag]

Thanks for starting this. What is here looks good but there are a few related things that need updating at the same time.

The code that is responsible for making a new browser version if the response collector reports a browser version that doesn't exist already always assumes Chrome.

The local simulated automation scheduler also assumes Chrome when it selects the relevant test plan report from a test plan version.

[gnarf]

Thanks for starting this. What is here looks good but there are a few related things that need updating at the same time.

thanks! glad you had the time to provide this input before it was forgotten! I'll get right on these changes!

[alflennik]

I looked over all the code, but mainly I focused on the transaction changes, which look to be really solid enhancements. I think we should merge this PR before #951 because I do expect some conflicts and I don't want them to block this PR.

We added the requested changes together

[howard-e]

Looks good to me! Went back over the newer changes since I've last reviewed and all seems in line to me.

Thanks as well for including the todo on what needs to happen when #958 is resolved.

edit: @gnarf I can't merge since there's a conflict to resolve so please feel free to do so after resolving that

[jugglinmike]

This patch implements gh-914.