Skip to content
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.

Sign up for GitHub

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

Jump to bottom

#2350. Update existing factory constructor tests. Part 1 #2353

Merged
merged 4 commits into from
Nov 28, 2023
Merged

#2350. Update existing factory constructor tests. Part 1 #2353

merged 4 commits into from
Nov 28, 2023

Conversation

sgrekhov
Copy link
Contributor

@sgrekhov sgrekhov commented Nov 6, 2023

No description provided.

@sgrekhov
Copy link
Contributor Author

@eernstg @chloestefantsova please review this PR. There are a lot of changes but most of them are "Update assertion text and add the same test for an enum"

eernstg
eernstg reviewed Nov 23, 2023
View reviewed changes
Copy link
Member

@eernstg eernstg left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure what happened here, it looks like there is a review that I wrote a long time ago, but didn't complete with a click on 'Review changes' (can't see any dates on 'pending' comments). Anyway, here's the missing click! ;-)

Language/Classes/Constructors/Factories/accessible_class_A02_t02.dart Outdated Show resolved Hide resolved
Language/Classes/Constructors/Factories/accessible_class_A02_t02.dart Outdated Show resolved Hide resolved
Language/Classes/Constructors/Factories/accessible_class_A02_t03.dart Outdated Show resolved Hide resolved
Language/Classes/Constructors/Factories/accessible_class_A02_t03.dart Outdated Show resolved Hide resolved
Language/Classes/Constructors/Factories/name_t06.dart Outdated Show resolved Hide resolved
Language/Classes/Constructors/Factories/name_t07.dart Outdated Show resolved Hide resolved
sgrekhov
sgrekhov commented Nov 23, 2023
View reviewed changes
Copy link
Contributor Author

@sgrekhov sgrekhov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you! Tests updated, please take another look

Language/Classes/Constructors/Factories/accessible_class_A02_t02.dart Outdated Show resolved Hide resolved
Language/Classes/Constructors/Factories/accessible_class_A02_t02.dart Outdated Show resolved Hide resolved
Language/Classes/Constructors/Factories/accessible_class_A02_t03.dart Outdated Show resolved Hide resolved
eernstg
eernstg reviewed Nov 27, 2023
View reviewed changes
Copy link
Member

@eernstg eernstg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! I commented on a bunch of little things.

Language/Classes/Constructors/Factories/accessible_class_A03_t01.dart Outdated Show resolved Hide resolved
Language/Classes/Constructors/Factories/name_t03.dart Outdated Show resolved Hide resolved
Language/Classes/Constructors/Factories/name_t06.dart Show resolved Hide resolved
Language/Classes/Constructors/Factories/redirecting_to_itself_t01.dart Outdated Show resolved Hide resolved
Language/Classes/Constructors/Factories/redirecting_to_itself_t02.dart Outdated Show resolved Hide resolved
Language/Classes/Constructors/Factories/return_type_A02_t07.dart Outdated Show resolved Hide resolved
Language/Classes/Constructors/Factories/return_type_A02_t08.dart Outdated Show resolved Hide resolved
Language/Classes/Constructors/Factories/return_type_A02_t09.dart Outdated Show resolved Hide resolved
Language/Classes/Constructors/Factories/return_type_A02_t10.dart Outdated Show resolved Hide resolved
Language/Classes/Constructors/Factories/return_type_A02_t10.dart Outdated Show resolved Hide resolved
@sgrekhov
Copy link
Contributor Author

Thank you! Updated. Please teake another look

eernstg
eernstg reviewed Nov 28, 2023
View reviewed changes
Copy link
Member

@eernstg eernstg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! We do have a few more typos, so let's get them done.

Language/Classes/Constructors/Factories/redirecting_to_itself_t04.dart Outdated Show resolved Hide resolved
Language/Classes/Constructors/Factories/redirecting_to_itself_t05.dart Outdated Show resolved Hide resolved
Language/Classes/Constructors/Factories/redirecting_to_itself_t07.dart Outdated Show resolved Hide resolved
Language/Classes/Constructors/Factories/name_t01.dart Outdated Show resolved Hide resolved
Language/Classes/Constructors/Factories/name_t02.dart Outdated Show resolved Hide resolved
Language/Classes/Constructors/Factories/redirecting_to_itself_t04.dart Outdated Show resolved Hide resolved
Language/Classes/Constructors/Factories/redirecting_to_itself_t06.dart Outdated Show resolved Hide resolved
Language/Classes/Constructors/Factories/redirecting_to_itself_t06.dart Outdated Show resolved Hide resolved
Language/Classes/Constructors/Factories/redirecting_to_itself_t07.dart Outdated Show resolved Hide resolved
@sgrekhov
Copy link
Contributor Author

Updated again, thank you. Please take another look

eernstg
eernstg approved these changes Nov 28, 2023
View reviewed changes
Copy link
Member

@eernstg eernstg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks!

@eernstg eernstg merged commit aab1a55 into dart-lang:master Nov 28, 2023
1 of 2 checks passed
copybara-service bot pushed a commit to dart-lang/sdk that referenced this pull request Dec 4, 2023
@sgrekhov @athomas
[co19] Roll co19 to 2ba55b45d85fece505a503dd12cf1c708825b74f
a3d5e1d 
2023-12-01 49699333+dependabot[bot]@users.noreply.github.com Bump actions/setup-java from 3.13.0 to 4.0.0 (dart-lang/co19#2410)
2023-12-01 sgrekhov22@gmail.com dart-lang/co19#2398. Update async tests to avoid false-positive results on web. Language and LanguageFeatures tests (dart-lang/co19#2407)
2023-12-01 sgrekhov22@gmail.com Fixes dart-lang/co19#2408. Fix roll failures (dart-lang/co19#2409)
2023-11-30 sgrekhov22@gmail.com dart-lang/co19#2398. Update asyncStart/End() to correspond SDK version. Replace asyncMultiTest (dart-lang/co19#2406)
2023-11-30 sgrekhov22@gmail.com dart-lang/co19#2398. Remove excessive async. Add explicit `void` (dart-lang/co19#2400)
2023-11-28 sgrekhov22@gmail.com dart-lang/co19#2350. Update existing factory constructor tests. Part 1 (dart-lang/co19#2353)
2023-11-28 sgrekhov22@gmail.com Fixes dart-lang/co19#2390. Add expected error to static_analysis_extension_types_A30_t02.dart (dart-lang/co19#2401)
2023-11-28 sgrekhov22@gmail.com Fixes dart-lang/co19#2399. Update expected errors locations for CFE (dart-lang/co19#2402)
2023-11-24 sgrekhov22@gmail.com dart-lang/co19#2388. Rename and reorder static_analysis_member_invocation_A06_t* tests (dart-lang/co19#2397)

Change-Id: Ie4b51caa12a9a0896c893cc02b099a07ef09fbd7
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/339560
Reviewed-by: Alexander Thomas <athom@google.com>
Reviewed-by: Erik Ernst <eernst@google.com>
Commit-Queue: Erik Ernst <eernst@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@eernstg eernstg eernstg approved these changes

@chloestefantsova chloestefantsova Awaiting requested review from chloestefantsova

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@sgrekhov @eernstg