fix #7740 type(test): Extend targeting integration tests

[b4handjr]

Because

• We currently check each targeting expression validates in the browser, but it is a very basic expression

This Commit

• Extends that test with additional fiends such as channel, locale, region and versions.

Also closes #6791

[b4handjr]

An example of what the full targeting expression will look like:

(browserSettings.update.channel == "nightly") && ('app.shield.optoutstudies.enabled'|preferenceValue) && (version|versionCompare('120.*') <= 0) && ((isFirstStartup && !('trailhead.firstrun.didSeeAboutWelcome'|preferenceValue))) && (version|versionCompare('100.!') >= 0) && (locale in ['en-CA']) && (region in ['CA'])

[jaredlockhart]

Oh awesome! Thanks for getting to this, increased coverage ftw 🎉

[jaredlockhart]

Ah yeah, unfortunately this isn't going to work the way I'd hoped, just testing this out locally. I tried injecting some invalid parts into one of the targeting constants like so

await targetingContext.evalWithDefault("(browserSettings.update.channel == 'release') && asdfasdf && ('app.shield.optoutstudies.enabled'|preferenceValue) && asdfasdf && (version|versionCompare('80.!') >= 0)")
false

but since the channel test comes first, and it doesn't match and returns false that short circuits the whole expression and returns false so we never get around to evaluating the invalid part. You can see if that if you put the invalid part at the start of the expression:

await targetingContext.evalWithDefault("asdfasdf && (browserSettings.update.channel == 'release') && ('app.shield.optoutstudies.enabled'|preferenceValue) && asdfasdf && (version|versionCompare('80.!') >= 0)")
null

So we're gonna have to do this a little differently. Instead of setting all the fields on every targeting test, we have to keep those with no additional audience fields so that only the targeting expression from the targeting constant evaluated. And then we should have a separate suite of tests that tests each of the audience fields individually and sets it and checks for validity so that we can check their emitted jexl expression in isolation.

Sorry to make you write all this up, but the other changes in here, like switching the desktop targeting tests to use the graphql endpoint instead of the ui, and fixing the name collision thing, are all good, so maybe a thing to do is just modify this to take out setting the additional fields but keep the graphql/naming part, land that, and then add the additional tests I just described above as a followup.

The other thing this highlights to me is we SHOULD do this #7464 even though it won't catch invalid dotted lookups, it would still catch the examples I'm showing above since it would evaluate each key in isolation.

[b4handjr]

Closing in favor of #7754