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

P2p merge #59

Merged
merged 100 commits into from
Mar 13, 2023
Merged

P2p merge #59

merged 100 commits into from
Mar 13, 2023

Conversation

ekigamba
Copy link
Contributor

@ekigamba ekigamba commented Jan 17, 2023

IMPORTANT: Where possible all PRs must be linked to a Github issue

Fixes #30

Type
Choose one: (Bug fix | Feature | Documentation | Testing | Code health | Release | Other)

Checklist

  • I have written Unit tests for any new feature(s) and edge cases for bug fixes
  • I have added any strings visible on UI components to the strings.xml file
  • I have run ./gradlew spotlessApply and ./gradlew spotlessCheck to check my code follows the project's style guide

Rkareko and others added 25 commits December 8, 2022 06:58
- Disconnect from wifi-direct when closing the activity
- Disconnect from wifi-direct before starting searching if the devices is already in a wifi-direct group
Fix display of cancel connection dialog
- Allow for 3 retries of client-server socket connections after 5 seconds
- Allow restarting the activity after an unrecoverable exception
@ekigamba
Copy link
Contributor Author

@Rkareko Kindly test the socket closing and disconnection after an exception. I tend to get a p2p BUSY error or an EADDRINUSE exception on this commit d199779 (#59)

@@ -372,14 +378,15 @@ class WifiDirectDataSharingStrategyTest : RobolectricTest() {

actionListenerSlot.captured.onSuccess()

Assert.assertTrue(ReflectionHelpers.getField(wifiDirectDataSharingStrategy, "paired"))
// TODO verify what happens on successPairingListener
/* Assert.assertTrue(ReflectionHelpers.getField(wifiDirectDataSharingStrategy, "paired"))
val actualCurrentDevice =
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do we need this for later and can we @ignore this with an issue to fix this later on

Copy link
Contributor

Choose a reason for hiding this comment

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

Documented on this issue

@@ -52,9 +60,15 @@ constructor(
var lastUpdatedAt =
P2PLibrary.getInstance().getReceiverTransferDao().receiveJson(currentManifest.dataType, data)

totalSentRecordCount += data!!.length()
totalReceivedRecordCount += data!!.length()
Timber.e(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Let's change these 2 lines to debug or verbose

override fun onStop() {
super.onStop()

dataSharingStrategy.onStop()
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Let's probably move this before super.onStop()

@@ -128,6 +135,7 @@ class P2PReceiverViewModel(
// Listen for incoming manifest
val receivedManifest =
dataSharingStrategy.receiveManifest(
// TODO: Fix this is null for some weird issue causing an NPE
device = dataSharingStrategy.getCurrentDevice()!!,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I guess we can remove this

}
}
)
}

fun processIncomingManifest() {
Timber.e("processIncomingManifest() started")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

let's remove this or change to info

}

@Ignore("Fix mocks not working in lazy function")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think we can create an issue for this and any other technical debt. It can be a candidate for CHT

Copy link
Contributor

Choose a reason for hiding this comment

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

Documented on #67

}

@Ignore("Fix mocks not working in lazy function")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think we can create an issue for this and any other technical debt. It can be a candidate for CHT

Copy link
Contributor

Choose a reason for hiding this comment

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

Documented on #67

fun `closeP2PScreen() calls view#finish()`() {
fun `closeP2PScreen() calls view#finish() when dataSharingStrategy#getCurrentDevice() is null`() {
every { dataSharingStrategy.getCurrentDevice() } returns null
p2PViewModel.closeP2PScreen()
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This does not seem to be complete. I don't seem to see any verification after the call to closeP2PScreen()

Copy link
Contributor

Choose a reason for hiding this comment

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

updated

}

@Test
fun `closeP2PScreen() calls view#finish() dataSharingStrategy#disconnect() is successful`() {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The test name is ambiguous or missing a conjunction between view#finish() and dataSharingStrategy#disconnect()

}

@Test
fun `closeP2PScreen() calls view#finish() dataSharingStrategy#disconnect() fails`() {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The test name is ambiguous or missing a conjunction between view#finish() and dataSharingStrategy#disconnect()

@Rkareko
Copy link
Contributor

Rkareko commented Mar 10, 2023

@Rkareko Kindly test the socket closing and disconnection after an exception. I tend to get a p2p BUSY error or an EADDRINUSE exception on this commit d199779 (#59)

@Rkareko Rkareko closed this Mar 10, 2023
@Rkareko Rkareko reopened this Mar 10, 2023
@Rkareko Rkareko merged commit 88a348e into main Mar 13, 2023
@Rkareko Rkareko deleted the p2p-merge branch March 13, 2023 05:14
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.

Handle failures in WifiDirectDataSharingStrategy class
2 participants