-
Notifications
You must be signed in to change notification settings - Fork 285
Fixing compile error in DeviceCompatibilityModeTestJavaSnippets #575
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
Conversation
Happened to notice a typo in the file, but it still didn't compile when I fixed the typo, so I made a second fix. The typo was a missing semicolon at the "import AppCompatActivity" line. But after I fixed that, I got an error complaining about `assertFalse(isLetterboxed(activity))` because `isLetterboxed` is expecting an AppCompatActivity but `activity` is a `MainActivity`. Changing `isLetterboxed` fixed the compile error and also meant we don't import AppCompatActivity so the original typo goes away. [shrug emoji] Is this fix okay?
|
What was the typo, Andrew? I don't see where it was fixed. Also, these files compiled previously. |
|
The typo was in line 19: (note the lack of a semicolon at the end of the first line). But when I fixed that typo, the file still wouldn't compile, because isLetterBoxed() expects to be passed an AppCompatActivity but it's actually being passed a MainActivity. When I changed the isLetterBoxed method to be passed a MainActivity, everything compiles -- but since the file no longer needs AppCompatActivity, AndroidStudio removed that "import AppCompatActivity" line, so the typo fix went away. ...just to confirm, when you submitted it, did you compile the AndroidTest module? I don't see how it could have compiled with that missing semicolon. |
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.
Update looks good here!
This never would have compiled - but it wasn't being caught on CI because this particular set of code underneath androidTest isn't being built and verified on CI.
I reported #576 for the bigger issue here of not validating snippets in instrumented test sources.
|
|
||
| // Method used by snippets. | ||
| public boolean isLetterboxed(AppCompatActivity activity) { | ||
| public boolean isLetterboxed(MainActivity activity) { |
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.
Why did you move to MainActivity instead of AppCompatActivity?
If the problem was a ; missing, this shouldn't be needed.
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.
As mentioned above -- when I added the semicolon I still couldn't compile, because isLetterBoxed() 's return value was being passed to something that expected a MainActivity. I don't know if this was the best way to fix that but it works and it doesn't change the code that's actually included in the docs, so 🤷
Thanks for submitting!
Happened to notice a typo in the file, but it still didn't compile when I fixed the typo, so I made a second fix.
The typo was a missing semicolon at the "import AppCompatActivity" line. But after I fixed that, I got an error complaining about
assertFalse(isLetterboxed(activity))becauseisLetterboxedis expecting an AppCompatActivity butactivityis aMainActivity. ChangingisLetterboxedfixed the compile error and also meant we don't import AppCompatActivity so the original typo goes away. [shrug emoji]Is this fix okay?