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

Add the model checking mode in Lincheck tests #2326

Merged
merged 6 commits into from
Nov 28, 2020

Conversation

ndkoval
Copy link
Member

@ndkoval ndkoval commented Oct 21, 2020

No description provided.

@ndkoval ndkoval force-pushed the lincheck-with-model-checking branch from df9bb17 to fc1bcb3 Compare November 3, 2020 11:34
@ndkoval
Copy link
Member Author

ndkoval commented Nov 3, 2020

@ndkoval ndkoval force-pushed the lincheck-with-model-checking branch from fc1bcb3 to c074deb Compare November 3, 2020 12:52
@ndkoval ndkoval requested a review from elizarov November 3, 2020 13:23
@ndkoval ndkoval marked this pull request as ready for review November 3, 2020 13:24
@@ -10,7 +10,7 @@ package kotlinx.coroutines.internal
* @suppress **This is unstable API and it is subject to change.**
*/
internal class Symbol(val symbol: String) {
override fun toString(): String = symbol
override fun toString(): String = "<$symbol>"
Copy link
Contributor

Choose a reason for hiding this comment

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

Why this is needed?

Copy link
Member Author

Choose a reason for hiding this comment

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

This is just more readable, to distinguish symbols from strings/enums

kotlinx-coroutines-core/build.gradle Show resolved Hide resolved
kotlinx-coroutines-core/build.gradle Show resolved Hide resolved
@@ -0,0 +1,7 @@
<component name="ProjectDictionaryState">
<dictionary name="ndkoval">
Copy link
Contributor

Choose a reason for hiding this comment

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

Please, give it some neutral name (like shared).

Copy link
Member Author

Choose a reason for hiding this comment

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

In IDEA each user has its own dictionary to reduce merge conflicts with the global dictionary, but all these per-user dictionaries are used by IDEA independently on the current user.

Copy link
Contributor

Choose a reason for hiding this comment

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

That's exactly my point. The dictionary that is committed to the report should not be your personal dictionary that is tied to your user-name, it shall be a common dictionary that is shared across the project.

Copy link
Member Author

Choose a reason for hiding this comment

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

That is not how IntelliJ IDEA works (yes, the behavior is counterintuitive). You cannot create a dictionary with a custom name -- it will be automatically cleaned up by IntelliJ IDEA. Instead, there are per-user dictionaries which can be updated by each user personally -- when you use IntelliJ IDEA all these per-user dictionaries are used, so that your IDEA will know about the words listed in ndkoval dictionary. The original motivation (provided by the IntelliJ IDEA team) is that in this case you will not have merge conflicts on the dictionary since it is split by users. I, personally, would not agree with this solution (at least, I spent an hour trying to understand the behavior), but that is how it works.

Copy link
Member Author

Choose a reason for hiding this comment

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

In conclusion,

  1. you cannot create dictionaries with custom names, and, in particular, a single global dictionary
  2. all per-user dictionaries are used in IDEA, independently of the current user

Copy link
Contributor

Choose a reason for hiding this comment

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

Of course, you can. I've committed a fix.

kotlinx-coroutines-core/build.gradle Outdated Show resolved Hide resolved
Copy link
Contributor

@elizarov elizarov 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 now (I've made a few extra tweaks). I'll wait for CI and merge.

@@ -0,0 +1,7 @@
<component name="ProjectDictionaryState">
<dictionary name="ndkoval">
Copy link
Contributor

Choose a reason for hiding this comment

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

Of course, you can. I've committed a fix.

@elizarov elizarov merged commit b13018d into develop Nov 28, 2020
@elizarov elizarov deleted the lincheck-with-model-checking branch November 28, 2020 12:03
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

2 participants