-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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.
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
fix: Build Sample App Tests
and integ_next_auth_authenticator_and_ssr_page
tests fix
#12655
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The name is not very good
Why not rename browser list to small
, medium
, large
?
wdyt?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was thinking something more like
browser_list_small
browser_list_medium
browser_list_large
minimal_browser_list_2
means there is anotherminimal_browser_list
but 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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/cli
version to16.2.10
. In attempt of fixing Build Sample App Testsinteg_next_auth_authenticator_and_ssr_page
to edge & chrome to align with e2e testsIssue #, if available
Description of how you validated changes
Checklist
yarn test
passesBy submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.