Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Unit test for a race condition in Device.js / disconnect() #78

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

nmasse-itix
Copy link
Contributor

This Pull Request is a proposal for a unit test exhibiting the behavior described in #73.

To implement this unit test, I had to replace in the actual code the call to removeListeners with a call to removeAllListeners.

In the unit tests, the EventEmitter implementation from nodejs is used as-is (no mock). But the removeListeners method is not part of that implementation. Only removeAllListeners is is officially part of the EventEmitter class (documentation).

With the following patch, the unit test is passing.

  /**
   * Connect to remote device
   */
  async connect () {
    const cb = (propertiesChanged) => {
      if ('Connected' in propertiesChanged) {
        const { value } = propertiesChanged.Connected
        if (value) {
          this.emit('connect', { connected: true })
        } else {
          this.emit('disconnect', { connected: false })
+         this.helper.removeAllListeners()
        }
      }
    }

    this.helper.on('PropertiesChanged', cb)
    await this.helper.callMethod('Connect')
  }

  /**
   * Disconnect remote device
   */
  async disconnect () {
    await this.helper.callMethod('Disconnect')
-   this.helper.removeAllListeners()
  }

@nmasse-itix
Copy link
Contributor Author

In the GitHub Actions, the unit tests are failing as expected.

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant