w3c / aria-at-app

Test runner and results reporter for ARIA-AT
http://aria-at.w3.org/
Other
35 stars 15 forks source link

Specify AtVersion for automation #1144

Closed stalgiag closed 2 months ago

stalgiag commented 3 months ago

Overview

This PR adds AtVersion specification for automated jobs to the app.

Dependent on aria-at-gh-actions-helper

Notes:

stalgiag commented 2 months ago

Since this is something that is currently only updated by code maintainers I thought it would be nice to have a single source of truth. Otherwise, we could potentially have dozens of related seeders down the road, and we would still need to maintain the constant array as well because an AtVersion might not be in the database when it becomes supported by automation. I have no indication that we will be automatically creating those AtVersions so that column will need to be determined on row creation. The wonkiness to get the virtual property seemed like a worthy tradeoff at the time but I could be missing complexity that will emerge down the road. I am open to changing it.

gnarf commented 2 months ago

The wonkiness

I think the root of "the wonk" here is trying to tie it into the Model at all. It might make more sense as a resolvers/AtVersion/supportedByAutomationResolver.js with a cache & stored strategy for the atNameById map.

It's always going to be a little wonk needing at least one async step, but I do think you could have a trio of methods exported from that resolver that were supportedByAutomationResolver(atVersion) which internally did await prepareAtNameMap(); return supportedByAutomationResolverSync(atVersion); - prepareAtNameMap would be setup on only "execute" the query once, and store the results in some var the resolver can use forever more.

Of course, this is it's own version of the jank, but does have a considerable advantage of at least not needing to do a roundtrip to postgres for every AtVersion in getAtVersionWithRequirements - and not having to do an extra roundtrip to postgres for every AtVersion exported in a gql query returning a bunch of them.

I'm not pushing super hard for this change, just putting it out there in case you also think it sounds "better" in some way

stalgiag commented 2 months ago

This is an interesting suggestion and I can see the value. I am definitely operating on the assumption that AtVersions will never be a huge collection but I think your solve would be better. I am not sure if it will make sense for me to prioritize that refactor and I'll have to check in with Carmen on that.

On another note: I just merged development in and the new tests are failing. Some work will need to be done to make these changes work with the new end-to-end tests. I will tackle the issue but just wanted to note this here!

stalgiag commented 2 months ago

Just noticed the new conflicts. Will handle today after my meetings

stalgiag commented 2 months ago

Okay! Going to merge this quick before another set of conflicts comes through 😅

Carmen made a follow-up task based on @gnarf's recommended approach. If there is time during this cycle I can take it on. Thanks all!