Fix IDE 324 : ClassCastException in DataListFragment and stabilize UI tests#5177
Conversation
|
@wslany Could you please add this PR issue to a Jira ticket? Apologies for raising the PR without linking one. If possible, may I create the ticket for this? |
wslany
left a comment
There was a problem hiding this comment.
@wslany Could you please add this PR issue to a Jira ticket? Apologies for raising the PR without linking one. If possible, may I create the ticket for this? Thanks!
Thanks for the follow-up PR. Yes, I would be grateful if you could create the ticket for it. If you can't yet create it, let me set it up for you. I'll also look into how you can get write access to our jira.
I checked the changes and also ran the focused FormulaEditorUndoTest suite locally.
The good part first: the ClassCastException root cause analysis makes sense, and the new popup-menu test interactions (inRoot(isPlatformPopup())) are a nice stability improvement.
One thing I would still like to revisit in the production code, though, is that this seems to fix the symptom rather than the underlying type mismatch. DataListFragment is still declared as T : UserData, while UserVariable is actually UserData. The new UserData<*> cast in showEditDialog() and the UserVariable special case in editItem() avoid the current crash, but the model is still inconsistent. I think it would be safer to widen the fragment’s generic type / value handling more explicitly instead of relying on local casts.
On the tests: when I ran FormulaEditorUndoTest on this PR branch by itself, most of the suite was still red because of the already-known undo-button visibility issue on current develop. That looks expected to me, since this follow-up branch is based on develop and does not include the production undo fix from #5171. So I would evaluate this PR mainly as a follow-up to #5171, not as a standalone green fix on current develop.
So from my side:
- the test helper changes look good
- the crash fix direction is reasonable
- but I would prefer one more refinement on the DataListFragment typing rather than keeping the current cast-based workaround as-is
Yes, thank you! I’d appreciate that. |
@wslany Thanks for the detailed feedback! I agree that the current approach relies on a cast-based workaround. I’ll take another pass to improve the type handling in DataListFragment and make the solution more robust and type-safe. |
There was a problem hiding this comment.
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
17a6f17 to
e14f088
Compare
|
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| assertEquals( | ||
| (ProjectManager.getInstance().currentProject.getUserVariable(VARIABLE_NAME).value as Number).toDouble(), | ||
| NEW_VARIABLE_VALUE.toDouble(), | ||
| 0.001 | ||
| ) |
There was a problem hiding this comment.
The assertEquals(expected, actual, delta) argument order is reversed here: the current-project value is passed as the expected parameter and the constant as the actual parameter. This won’t usually fail the test, but it makes assertion failure messages misleading. Swap the first two arguments so NEW_VARIABLE_VALUE.toDouble()/VARIABLE_VALUE.toDouble() is the expected value and the project value is the actual.
| assertEquals( | ||
| (ProjectManager.getInstance().currentProject.getUserVariable(VARIABLE_NAME).value as Number).toDouble(), | ||
| VARIABLE_VALUE.toDouble(), | ||
| 0.001 | ||
| ) |
There was a problem hiding this comment.
Same as above: assertEquals(expected, actual, delta) has expected/actual swapped, which makes failures harder to interpret. Pass VARIABLE_VALUE.toDouble() as expected and the project value as actual.



JIRA TICKET - https://catrobat.atlassian.net/browse/IDE-324
Description:
This pull request addresses a
ClassCastExceptioninDataListFragmentand resolves test regressions within theFormulaEditorUndoTest. Additionally, it stabilizes UI interactions for Espresso tests involving platform popups.Note: This pull request is a continuation of (#5171)
Detailed Changes
1. Fixed
ClassCastExceptioninDataListFragment.ktDataListFragmenthad a type-safety issue when trying to update user variables, which could lead to aClassCastExceptionor incorrect data types being saved.UserVariabletypes ineditItem. It now intelligently tries to parse the input value as aDoublefirst (for numeric variables) and falls back to aStringif it's not a number. This ensures the underlying data model remains consistent.2. Resolved
FormulaEditorUndoTestRegressionscloseSoftKeyboard()calls to prevent the soft keyboard from obstructing the view during automated tests. Updated numeric assertions to use a delta comparison (assertEquals(expected, actual, 0.001)), making the tests more robust against minor precision differences in floating-point math.3. Stabilized UI Test Interactions
UserDataItemRVInteractionWrapper.javato explicitly search for these buttons within the platform popup root (isPlatformPopup()). This prevents common "No Matching View" errors in UI tests.Files Modified:
DataListFragment.kt: UpdatededitFieldandeditItemlogic.FormulaEditorUndoTest.java: Improved test stability and assertions.UserDataItemRVInteractionWrapper.java: Fixed popup menu interaction logic.Checklist for this pull request
Please review the contributing guidelines and wiki pages of this repository.