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.

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

await a missed test failure, fix broken test #3245

Merged
merged 2 commits into from Feb 16, 2022
Merged

Conversation

natebosch
Copy link
Member

@natebosch natebosch commented Feb 15, 2022

This test should be failing but the error is lost because of the way the
async failure interacts with the runBuilder error handling zone.
dart-lang/test#1670

Use await expectLater instead of await expect to ensure the error is
not lost.

Change the setup for the test to include two imports instead of 1 import
and a local class definition. Move the usage to an annotation so that it
is easy to find from the library using it.

This test should be failing but the error is lost because `throwsA` does
not force the test to wait until the condition is checked.
dart-lang/test#1670

Fix the pattern to use `await expectLater` instead of `await expect` so
the failure will show up. Skip the test for now.
@natebosch
Copy link
Member Author

This test was introduced during the migration to null safety, I'm not 100% sure I understand the value it brings.

@jakemac53 - do you know the intention behind this test?

@jakemac53
Copy link
Contributor

I don't fully understand the test, no. I am not sure why the asset wouldn't be resolvable?

@natebosch
Copy link
Member Author

@simolus3 - do you recall the intent behind this test?

@simolus3
Copy link
Contributor

I'm not sure, but I think it may have been something about MultiplyDefinedElements not having a source? This part from the same commit looks related

@@ -263,12 +272,17 @@ class AnalyzerResolver implements ReleasableResolver {
 
   @override
   Future<AssetId> assetIdForElement(Element element) async {
-    final uri = element.source.uri;
-    if (!uri.isScheme('package') && !uri.isScheme('asset')) {
+    final source = element.source;
+    if (source == null) {
       throw UnresolvableAssetException(
-          '${element.name} in ${element.source.uri}');
+          '${element.name} does not have a source');
+    }
+
+    final uri = source.uri;
+    if (!uri.isScheme('package') && !uri.isScheme('asset')) {
+      throw UnresolvableAssetException('${element.name} in ${source.uri}');
     }
-    return AssetId.resolve('${element.source.uri}');
+    return AssetId.resolve('${source.uri}');
   }
 }

Looks like we'd crash before, I've realized that with the null-safety migration and changed it to throw the exception instead?

@jakemac53
Copy link
Contributor

I'm not sure, but I think it may have been something about MultiplyDefinedElements not having a source? This part from the same commit looks related

That makes sense, but I don't get why this is a MultiplyDefinedElement in this case? It seems to be grabbing it directly from the imported library?

@simolus3
Copy link
Contributor

simolus3 commented Feb 15, 2022

Yeah looking at the test it seems like I got it wrong. I also couldn't figure out an easy way to get a MultiplyDefinedElement now, so maybe the test should just be removed?

@jakemac53
Copy link
Contributor

I think you could try defining the same class twice in the same library - no need for any imported library?

@simolus3
Copy link
Contributor

simolus3 commented Feb 15, 2022

I've tried that, but still no luck. With the following library, the analyzer returns the first class for getType('SomeClass') and exportNamespace.get('SomeClass'):

class SomeClass {}
class SomeClass {}

It looks like it's the same when I lookup the element from the public namespace when importing two different libraries both defining SomeClass.

@jakemac53
Copy link
Contributor

Interesting, I suppose we can just go without testing this behavior, but we should leave in the code to throw if we don't have a source.

Use an annotation that could resolve to a class from one of two imports.
@natebosch
Copy link
Member Author

I think I found a pattern that can give us an element without a source. @jakemac53 PTAL

@natebosch natebosch changed the title await a missed test failure, skip broken test await a missed test failure, fix broken test Feb 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants