Skip to content

Conversation

vitvakatu
Copy link
Contributor

Overview


See: https://jira.bf.local/browse/ECR-3204

Definition of Done

  • There are no TODOs left in the code
  • Change is covered by automated tests
  • The coding guidelines are followed
  • Public API has Javadoc
  • Method preconditions are checked and documented in the Javadoc of the method
  • Changelog is updated if needed (in case of notable or breaking changes)
  • The continuous integration build passes

@vitvakatu vitvakatu added do not merge ⛔️ work-in-progress 👷‍♂️ Do not expect reviewers to provide any feedback on WIP PRs — please ask for it explicitly! labels May 30, 2019
@coveralls
Copy link

coveralls commented May 30, 2019

Coverage Status

Coverage remained the same at 85.398% when pulling 5ba4597 on vitvakatu:tls_executor into 3658d72 on exonum:master.

}

/// Executes a provided closure, making sure that the current thread
/// is attached to the JVM. Additionally ensures that local object references freed after call.
Copy link
Contributor

Choose a reason for hiding this comment

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

are freed after call?

Copy link
Contributor

Choose a reason for hiding this comment

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

also below

#[test]
fn single_thread() {
test_single_thread(&*EXECUTOR);
test_single_thread(EXECUTOR.clone());
Copy link
Contributor

Choose a reason for hiding this comment

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

Probably not single with the guys below in the same file 🙃

I think we can leave it as is, but please inspect these tests to see if they need to be updated or extended.

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

At least nested_attach cannot reliably live in the same source file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually it can, but it will be unable to check if the thread is detached correctly after the outer-most scope is finished. Other tests a totally fine to live together.

}

#[test]
fn nested_attach() {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we shall keep it because it verifies the executor implementation allowing recursive with_attached, which some implementations might not permit.

@vitvakatu vitvakatu removed do not merge ⛔️ work-in-progress 👷‍♂️ Do not expect reviewers to provide any feedback on WIP PRs — please ask for it explicitly! labels Jun 12, 2019
@dmitry-timofeev dmitry-timofeev merged commit e2419e4 into exonum:master Jun 12, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

4 participants