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

Fixed #4396, #4883 : Add CI checks to ensure Kotlin files , Layout xml files are only referencing colors from component_colors.xml #4882

Merged
merged 29 commits into from
Apr 6, 2023
Merged
Show file tree
Hide file tree
Changes from 17 commits
Commits
Show all changes
29 commits
Select commit Hold shift + click to select a range
20bea63
Add CI checks to ensure layout xml files are only referencing colors …
MohitGupta121 Feb 24, 2023
c42087f
Add CI checks to ensure kotlin files are only referencing colors from…
MohitGupta121 Feb 24, 2023
b0d8cbe
Convert 4 checks into 1
MohitGupta121 Feb 25, 2023
aad4625
Merge branch 'develop' into color_ci_checks_for_xml_files
MohitGupta121 Feb 27, 2023
29f6c3d
Regex check added for Custom views
MohitGupta121 Feb 28, 2023
bb6a60c
Testing Activity and Fragments Test
MohitGupta121 Feb 28, 2023
3394292
Testing Activity and Fragments Test
MohitGupta121 Feb 28, 2023
2e533df
Testing Activity and Fragments Test
MohitGupta121 Feb 28, 2023
71e75ec
Added ViewPresenter test in regex
MohitGupta121 Feb 28, 2023
8182595
Merge branch 'develop' into color_ci_checks_for_xml_files
MohitGupta121 Mar 3, 2023
0edacb4
Merge branch 'develop' into color_ci_checks_for_xml_files
MohitGupta121 Mar 11, 2023
745a937
Merge branch 'develop' into color_ci_checks_for_xml_files
MohitGupta121 Mar 14, 2023
5dab604
Merge branch 'develop' of https://github.com/oppia/oppia-android into…
MohitGupta121 Mar 15, 2023
9ef70ee
Added RegexPatternValidationCheckTest for xml layouts from component …
MohitGupta121 Mar 15, 2023
8a8aa38
Added RegexPatternValidationCheckTest for Kotlin files to test compon…
MohitGupta121 Mar 15, 2023
e90efa4
Added exempted_file_name which need to fix in follow up PR.
MohitGupta121 Mar 15, 2023
18910c2
Merge branch 'develop' into color_ci_checks_for_xml_files
MohitGupta121 Mar 21, 2023
a1d526d
Merge branch 'develop' into color_ci_checks_for_xml_files
MohitGupta121 Mar 22, 2023
8a1126c
Added @Test annotations
MohitGupta121 Mar 22, 2023
028e4c4
Merge branch 'develop' into color_ci_checks_for_xml_files
MohitGupta121 Mar 24, 2023
0998d95
Run tests in new file of kotlin and layout
MohitGupta121 Mar 25, 2023
1559ad1
Merge branch 'develop' into color_ci_checks_for_xml_files
MohitGupta121 Apr 2, 2023
3247cc0
Fixed RegexPatternValidationCheckTest Failures
MohitGupta121 Apr 4, 2023
4394e83
Merge remote-tracking branch 'origin/color_ci_checks_for_xml_files' i…
MohitGupta121 Apr 4, 2023
22e5687
Fixed 100 char lint
MohitGupta121 Apr 4, 2023
2c12dab
Minor formatting
MohitGupta121 Apr 4, 2023
d842485
Fixed Kotlin Regex failure
MohitGupta121 Apr 4, 2023
8c4e7d4
Fixed Kotlin Regex failure
MohitGupta121 Apr 4, 2023
2184003
Fixed Sorted in RegexPatternValidationCheck
MohitGupta121 Apr 4, 2023
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
46 changes: 46 additions & 0 deletions scripts/assets/file_content_validation_checks.textproto
Original file line number Diff line number Diff line change
Expand Up @@ -339,6 +339,52 @@ file_content_checks {
prohibited_content_regex: "name=.+(?<!_color).>@|name=.*[A-Z].*>@"
failure_message: "All color declarations in component_color.xml and color_palette.xml should end with _color suffix following snake_case naming convention."
}
file_content_checks {
file_path_regex: "app/src/main/res/layout.*?/.+?\\.xml"
prohibited_content_regex: "@color/(?!component_color_).+"
failure_message: "Only colors from component_colors.xml may be used in layouts."
exempted_file_name: "app/src/main/res/layout/test_margin_bindable_adapter_activity.xml"
exempted_file_name: "app/src/main/res/layout/profile_reset_pin_activity.xml"
exempted_file_name: "app/src/main/res/layout/activity_input_interaction_view_test.xml"
exempted_file_name: "app/src/main/res/layout/profile_list_learner_id_item.xml"
exempted_file_name: "app/src/main/res/layout/mark_topics_completed_topic_view.xml"
exempted_file_name: "app/src/main/res/layout/onboarding_slide_final.xml"
exempted_file_name: "app/src/main/res/layout/home_test_activity.xml"
exempted_file_name: "app/src/main/res/layout/coming_soon_topic_view.xml"
exempted_file_name: "app/src/main/res/layout/drawer_fragment.xml"
exempted_file_name: "app/src/main/res/layout/mark_topics_completed_fragment.xml"
exempted_file_name: "app/src/main/res/layout/mark_stories_completed_story_summary_view.xml"
exempted_file_name: "app/src/main/res/layout/mark_chapters_completed_fragment.xml"
exempted_file_name: "app/src/main/res/layout/walkthrough_topic_summary_view.xml"
exempted_file_name: "app/src/main/res/layout/general_availability_upgrade_notice_dialog_content.xml"
exempted_file_name: "app/src/main/res/layout/topic_fragment.xml"
exempted_file_name: "app/src/main/res/layout/profile_list_device_id_item.xml"
exempted_file_name: "app/src/main/res/layout/profile_list_fragment.xml"
exempted_file_name: "app/src/main/res/layout/profile_picture_activity.xml"
exempted_file_name: "app/src/main/res/layout/splash_activity.xml"
exempted_file_name: "app/src/main/res/layout/mark_chapters_completed_chapter_summary_view.xml"
exempted_file_name: "app/src/main/res/layout/hints_and_solution_fragment.xml"
exempted_file_name: "app/src/main/res/layout/topic_practice_subtopic.xml"
exempted_file_name: "app/src/main/res/layout/walkthrough_topic_list_fragment.xml"
exempted_file_name: "app/src/main/res/layout/cellular_data_dialog.xml"
exempted_file_name: "app/src/main/res/layout/concept_card_fragment.xml"
exempted_file_name: "app/src/main/res/layout/my_downloads_fragment.xml"
exempted_file_name: "app/src/main/res/layout/add_profile_activity.xml"
exempted_file_name: "app/src/main/res/layout/beta_notice_dialog_content.xml"
exempted_file_name: "app/src/main/res/layout/exploration_test_activity.xml"
exempted_file_name: "app/src/main/res/layout/multiple_choice_interaction_items.xml"
exempted_file_name: "app/src/main/res/layout-land/onboarding_slide_final.xml"
exempted_file_name: "app/src/main/res/layout-sw600dp-land/onboarding_slide_final.xml"
exempted_file_name: "app/src/main/res/layout-sw600dp/profile_list_fragment.xml"
exempted_file_name: "app/src/main/res/layout-sw600dp-port/onboarding_slide_final.xml"
}
file_content_checks {
file_path_regex: "app/src/main/java/org/oppia/android/app.?/.+(ActivityPresenter|FragmentPresenter|ViewPresenter|Activity|Fragment|View)\\.kt"
prohibited_content_regex: "R.color.(?!component_color_).+"
failure_message: "Only colors from component_colors.xml may be used in Kotlin Files (Activities, Fragments, Views and Presenters)."
exempted_file_name: "app/src/main/java/org/oppia/android/app/profile/ProfileChooserFragmentPresenter.kt"
exempted_file_name: "app/src/main/java/org/oppia/android/app/profile/AddProfileActivityPresenter.kt"
}
file_content_checks {
file_path_regex: "app/src/main/res/values/color_defs.xml"
prohibited_content_regex: "name=.+(?<=color).>#|name=.*[A-Z].*>#"
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -164,6 +164,11 @@ class RegexPatternValidationCheckTest {
"Only colors from color_palette.xml may be used in component_colors.xml."
private val doesNotReferenceColorFromColorDefs =
"Only colors from color_defs.xml may be used in color_palette.xml."
private val doesNotReferenceColorFromComponentColorInLayouts =
"Only colors from component_colors.xml may be used in layouts."
private val doesNotReferenceColorFromComponentColorInKotlinFiles =
"Only colors from component_colors.xml may be used in Kotlin Files (Activities, Fragments," +
"Views and Presenters)."
private val doesNotUseWorkManagerGetInstance =
"Use AnalyticsStartupListener to retrieve an instance of WorkManager rather than fetching one" +
" using getInstance (as the latter may create a WorkManager if one isn't already present, " +
Expand Down Expand Up @@ -2153,6 +2158,98 @@ class RegexPatternValidationCheckTest {
)
}

fun testFileContent_xmlLayouts_includesNonColorComponentReferences_fileContentIsNotCorrect() {
BenHenning marked this conversation as resolved.
Show resolved Hide resolved
val prohibitedContent =
"""
android:textColor="@color/component_color_shared_primary_text_color"
android:textColor="@color/color_defs_shared_primary_text_color"
android:textColor="@color/color_palette_primary_text_color"
android:textColor="#000000"
android:background="@color/component_color_shared_primary_text_color"
android:background="@color/color_defs_shared_primary_text_color"
android:background="@color/color_palette_primary_text_color"
android:background="#000000"
app:tint="@color/component_color_shared_primary_text_color"
app:tint="@color/color_defs_shared_primary_text_color"
app:tint="@color/color_palette_primary_text_color"
app:tint="#000000"
app:strokeColor="@color/component_color_shared_primary_text_color"
app:strokeColor="@color/color_defs_shared_primary_text_color"
app:strokeColor="@color/color_palette_primary_text_color"
app:strokeColor="#000000"
app:cardBackgroundColor="@color/component_color_shared_primary_text_color"
app:cardBackgroundColor="@color/color_defs_shared_primary_text_color"
app:cardBackgroundColor="@color/color_palette_primary_text_color"
app:cardBackgroundColor="#000000"
android:background="@color/component_color_shared_primary_text_color"
android:background="@color/color_defs_shared_primary_text_color"
android:background="@color/color_palette_primary_text_color"
android:background="#000000"
""".trimIndent()
tempFolder.newFolder("testfiles", "app", "src", "main", "res", "layout")
val stringFilePath = "app/src/main/res/layout.*?/.+?\\.xml"
tempFolder.newFile("testfiles/$stringFilePath").writeText(prohibitedContent)

val exception = assertThrows(Exception::class) {
runScript()
}

// Verify that all patterns are properly detected & prohibited.
assertThat(exception).hasMessageThat().contains(REGEX_CHECK_FAILED_OUTPUT_INDICATOR)
assertThat(outContent.toString().trim())
.isEqualTo(
"""
$stringFilePath:2: $doesNotReferenceColorFromComponentColorInLayouts
$stringFilePath:3: $doesNotReferenceColorFromComponentColorInLayouts
$stringFilePath:4: $doesNotReferenceColorFromComponentColorInLayouts
$stringFilePath:6: $doesNotReferenceColorFromComponentColorInLayouts
$stringFilePath:7: $doesNotReferenceColorFromComponentColorInLayouts
$stringFilePath:8: $doesNotReferenceColorFromComponentColorInLayouts
$stringFilePath:10: $doesNotReferenceColorFromComponentColorInLayouts
$stringFilePath:11: $doesNotReferenceColorFromComponentColorInLayouts
$stringFilePath:12: $doesNotReferenceColorFromComponentColorInLayouts
$stringFilePath:14: $doesNotReferenceColorFromComponentColorInLayouts
$stringFilePath:15: $doesNotReferenceColorFromComponentColorInLayouts
$stringFilePath:16: $doesNotReferenceColorFromComponentColorInLayouts
$stringFilePath:18: $doesNotReferenceColorFromComponentColorInLayouts
$stringFilePath:19: $doesNotReferenceColorFromComponentColorInLayouts
$stringFilePath:20: $doesNotReferenceColorFromComponentColorInLayouts
$stringFilePath:22: $doesNotReferenceColorFromComponentColorInLayouts
$stringFilePath:23: $doesNotReferenceColorFromComponentColorInLayouts
$stringFilePath:24: $doesNotReferenceColorFromComponentColorInLayouts
$wikiReferenceNote
""".trimIndent()
)
}

fun testFileContent_kotlinFiles_includesNonColorComponentReferences_fileContentIsNotCorrect() {
val prohibitedContent =
"""
R.color.component_color_shared_activity_status_bar_color
R.color.color_def_avatar_background_1
R.color.color_palette_primary_color
""".trimIndent()
tempFolder.newFolder("testfiles", "app", "src", "main", "java", "org", "oppia", "android")
val stringFilePath = "app/src/main/java/org/oppia/android/app.?/." +
BenHenning marked this conversation as resolved.
Show resolved Hide resolved
"+(ActivityPresenter|FragmentPresenter|ViewPresenter|Activity|Fragment|View)\\\\.kt"
tempFolder.newFile("testfiles/$stringFilePath").writeText(prohibitedContent)

val exception = assertThrows(Exception::class) {
runScript()
}

// Verify that all patterns are properly detected & prohibited.
assertThat(exception).hasMessageThat().contains(REGEX_CHECK_FAILED_OUTPUT_INDICATOR)
assertThat(outContent.toString().trim())
.isEqualTo(
"""
$stringFilePath:2: $doesNotReferenceColorFromComponentColorInKotlinFiles
$stringFilePath:3: $doesNotReferenceColorFromComponentColorInKotlinFiles
$wikiReferenceNote
""".trimIndent()
)
}

@Test
fun testFileContent_colorPalette_referencesColorFromColorDefs_fileContentIsCorrect() {
val prohibitedContent =
Expand Down