fix: Build Sample App Tests and integ_next_auth_authenticator_and_ssr_page tests fix
#12655
Conversation
.github/canary-config/canary-all.yml
Outdated
| @@ -305,4 +309,4 @@ tests: | |||
| category: auth | |||
| sample_name: [auth-ssr] | |||
| spec: auth-ssr | |||
| browser: *minimal_browser_list | |||
| browser: *minimal_browser_list_2 | |||
There was a problem hiding this comment.
Since this is the only test running on minimal_browser_list_2, should we just run this in chrome for now to keep it in sync with e2e.
or
browser: [chrome, edge]
maybe later down the road, minimal_browser_list can be replaced with extended_browser_list depending on all the browsers we want to support
There was a problem hiding this comment.
I feel even if there only one test running and with this particular test being a special needs test we need to respect it and let it be there. If you can replace minimal_browser_list with extended_browser_list then we can also replace minimal_browser_list_2 with extended_browser_list. Let me know if you need any further explanation.
Question: But why only chrome, why not include edge any reason? and why does it has to be in sync with e2e if it is in sync why do we need canaries in first place?
There was a problem hiding this comment.
I was thinking along the lines of canary being more like a subset of e2e. I think it makes sense to run this on edge as well if required. Maybe we can run this test on edge on e2e and canary. wdyt
Also since minimal_browser_list_2 was only used by this test for now I was wondering if we could just specify browser: [chrome, edge] instead of creating the new variable. I don't have a strong opinion though
cc @elorzafe
There was a problem hiding this comment.
We never know if we have some other test this kindaaa variable and need to be assigned then we need to create again and replace it in multiple places . It would be odd in multiple places if it will not be in consistent in with all other places where they have something of pointers and now an array []. Curious to know your opinion here. May be people in CT or EST might know the future. They are ahead than us. @ashwinkumar6
There was a problem hiding this comment.
I think we mostly try to use minimal_browser_list in all places. Except some special tests which only run on specific browsers instead of the entire minimal_browser_list. For this we add the browser list inline
(This is something we currently do for e2e)
I think adding browser list inline might also help identify these special tests in future and troubleshoot why it doesn't work for a particular browser. So i think it's fine if it's slightly inconsistent
There was a problem hiding this comment.
@ashwinkumar6 can you takē a l00k at it now please 🙏🏼
.github/canary-config/canary-all.yml
Outdated
| @@ -2,6 +2,10 @@ minimal_browser_list: &minimal_browser_list | |||
| - chrome | |||
| - firefox | |||
|
|
|||
| minimal_browser_list_2: &minimal_browser_list_2 | |||
There was a problem hiding this comment.
The name is not very good
Why not rename browser list to small, medium, large?
wdyt?
There was a problem hiding this comment.
OOOOOhh I though minimal_small and minimal_medium and minimum_ large does not make Any sort of sense is understanding Francisco Rodriguez @elorzafe
There was a problem hiding this comment.
I was thinking something more like
browser_list_smallbrowser_list_mediumbrowser_list_large
minimal_browser_list_2means there is anotherminimal_browser_listbut doesn't give me any information related to that parameter.
If names doesn't make sense I would prefer to list the values inline. The idea is to understand whats going in a simple manner.
There was a problem hiding this comment.
@elorzafe, can you takē a l00k at it now Francisco please 🙏🏼
…ssr_page` tests fix (aws-amplify#12655) fix: Build Sample App Tests and integ_next_auth_authenticator_and_ssr_page tests
Description of changes
@angular/cliversion to16.2.10. In attempt of fixing Build Sample App Testsinteg_next_auth_authenticator_and_ssr_pageto edge & chrome to align with e2e testsIssue #, if available
Description of how you validated changes
Checklist
yarn testpassesBy submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.