Anki-Android
Anki-Android copied to clipboard
ankidroid
Add lint for `Build.VERSION.SDK_INT` in test
I made a mistake that was not seen by the reviewers. Essentially Build.VERSION.SDK_INT is 0 unless we use @Config(sdk = [21, 26]) or targetApi. So, I would love a linter that ensure that if either this constant or CompatHelper is used, then we configure the SDK.
It may be possible to simply declare that an illegal string to have in the code files and use a generic test that already exists to ensure specific strings are never used
Sounds good to me! We should confirm with @mikehardy that this is a path we're happy to go down, but it seems sensible to me.
@ayushsoni916 The task's already assigned, it'd be best to wait a while before requesting it (it's only been a week), there's no rush in Open Source.
You'd be best to select a different issue
We allow compathhelper to use Build.VERSION.SDK_INT, while using Build.VERSION.SDK_INT in other places will throw lint error. Now we have two problems:
1.We can't pass the test: Code Quality Checks / Lint Debug (pull_request) .But we only add a new lint and a lint Test. May be it is because the change of lint.rules affect the normal code, but we have checked the new link rule that we uploaded should not affect any outside code for now. We would like to know if there is a solution.
2.We have some unclear points about the remaining problems. We are not sure about the location of @Config(sdk = [21, 26]) and targetApi in the project (at present, we guess that they are in Config. kt and NotificationChannelTest.java respectively, but we are not sure). And we don't know how @Config(sdk = [21, 26]) and targetApi interact with Build.VERSION.SDK_INT. We hope to get more detailed description of the problem. Thank you!
Thank you very much for attacking this issue @HuWeiyy . I beg your pardon, I now realize how unclear I was.
As the issue name indicate, the link should only apply to usage of SDK_INT in tests. It should NOT apply to any file in the main folder. The issue is that SDK_INT value is 0 when we run test, which mean that we are not actually testing what we were hoping to test if we use this constant. However, if you targetApi or configure the SDK version, then SDK_INT will not be 0, it will have the value you targeted/configured, and the test will work as you expect.
Are you a team? Do you use "we" to speak of the person who has the @HuWeiyy account? I'm confused by your message, as I don't understand who else is included in your "we" in "we don't know how config and target api interact..."
Hello 👋, this issue has been opened for more than 2 months with no activity on it. If the issue is still here, please keep in mind that we need community support and help to fix it! Just comment something like still searching for solutions and if you found one, please open a pull request! You have 7 days until this gets closed automatically
![github-actions[bot] avatar](https://avatars.githubusercontent.com/in/15368?v=4 "Published by a pub.dev verified publisher"){kind=small}
@ayushsoni916 It seems the task was entirely abandoned. If you're still interested, please I'd welcome any help here
Please clarify, do you want to detect only direct usages of SDK_INT and CompatHelper in tests, or also cases when tests call methods that use them inside?
That's a great question @RSBat, thanks a lot. And nice to meet you
Ideally, I'd love if you can detect any use, even indirect, because it will clearly have a wrong value. If you're able to do that without making tests more complex or costly, it would be wonderful. I can imagine you could do that by using a MockStatic and ensuring that a call to get SDK_INT raise an exception during tests; it may be worth testing it, but there is a risk that either it breaks too many tests or that it increases the time to run tests too much for it to be acceptable.
As a first step anyway, I'd consider that any use inside the test class would be already a progress, as, at least, it would ensure that we detect the error similar to the one leading to the creation of this issue
That's an interesting idea, but looks impossible - there's no way to mock primitive types or observe field accesses.
Actually the best option might be lint check similar to WrongThreadInterproceduralDetector. Though it uses analyzeCallGraph which is marked as "Highly experimental as well as resource intensive!".
I beg your pardon, what do you call a "primitive type"? Because as far as I now, in Java, it corresponds such as int, long, and so on. But here, I'd have mocked Build.VERSION.
Anyway, you seems to be right, at least Mockito's mockStatic can't deal with final value, so we at least can't use the same class mocking infrastructure here.
Can I make a request, @RSBat? We don't know each other yet, and right now it's impossible for me to separate "looks impossible" from someone who really knows JVM and have ideas why it is actually impossible, from someone who would just state this because this is not the way Java usually works and had not heard of mockStatic or other way to change internal behavior in a non standard way. So my request would be to maybe put more explanation (maybe a link) explaining why you know something is impossible.
May I suggest a simple alternative. Have a utils method val getSdkInt() = Build.SDK.SDK_INT, forbids to use SDK_INT everywhere else, and mock this one
It was a very simple conclusion - I couldn't find anything in the javadocs or on the internet.
Actual reason is best described here. Accessing a static field is a single getstatic instruction in the accessing function. Intercepting the read requires modification to the bytecode of accessing functions. So not impossible, but not part of mockito. Here's a more complete example of doing it.
thank you very much. While you're at it. Any opinion about the last suggestion? I'd expect this one to be relatively easy to implement
Yes, it's a lot easier, but it won't affect third party libraries. This should be fine as we should not be testing their behavior anyways.
Another thing to keep in mind is applying it to every test:
- JUnit 4 uses
TestRules while JUnit5 uses extensions and has limited support for rules - There are no global rules in junit 4, a comprehensive review is available in https://github.com/junit-team/junit4/issues/1219
Finally, I find this solution a bit ugly, but I don't have better alternatives.
Hello 👋, this issue has been opened for more than 2 months with no activity on it. If the issue is still here, please keep in mind that we need community support and help to fix it! Just comment something like still searching for solutions and if you found one, please open a pull request! You have 7 days until this gets closed automatically
Hello 👋, this issue has been opened for more than 2 months with no activity on it. If the issue is still here, please keep in mind that we need community support and help to fix it! Just comment something like still searching for solutions and if you found one, please open a pull request! You have 7 days until this gets closed automatically