More tests, workflows PR comment for feature test by varunghat · Pull Request #405 · phoible/dev

varunghat · 2026-08-20T22:57:44Z

Added the following regression tests:

Clicks and modifiers get typed as "M", which caused some issues previously with clicks and diacritics getting swapped where they shouldn't. Added a test to make sure the behaviour is as intended.
c-cedilla is a special case handled in denorm(), as well as 20 special-cases for features. Added a test to ensure these are not changed

Added the following new test

test-cross-inventory-integrity.R checks if
- any (Source, Glottocode, LanguageName, SpecificDialect, BibtexKey) repeats with the same InventoryID , which implies there's two languages with the same InventoryID
- Checks if all BibtexKey map to a valid Bibtex entry in phoible-references.bib
- Checks if each InventoryID in bibtex mapping file is present in the final phoible.csv and vice-versa

Added the following extra test to test-inventory-metadata.R

Check if the ISOcodes present in glottolog match the ISOcodes present in phoible (With certain exceptions where glottolog keeps the older ISOcodes)

Excluded test-phonemes.R from github workflows test suite, but added it as a separate job which writes the output as a comment in the PR

Added tasks to pixi.toml (exactly the same as #398)

…s and add-features

…sts fail

varunghat · 2026-08-21T12:43:00Z

Split into two workflows now (Since PR from forked repo does not have write permissions)

First workflow runs the Feature test (test-phonemes.R) first and stores data. Then runs the tests suite.
Second workflow runs on completion of the first, and writes the PR comment.

Verified the workflow on my forked branch, PR comment works with PR from a forked repo. Will work once the new workflow file gets added to master.

Current test fails are the 5 ISOcodes that do not match with glottolog (More info in this PR that fixes them: #403)

drammock · 2026-08-22T19:43:12Z

    runs-on: ubuntu-latest
+    permissions:
+      contents: read
+      actions: write


why does this job need actions: write?

Job does not need actions: write. I wrongly assumed writing file needed it. Removed in 520b592. Tested on separate fork.

drammock · 2026-08-22T19:43:49Z

        run: |
-          Rscript -e "testthat::test_dir('.', stop_on_failure=TRUE)"
+            Rscript -e "testthat::test_dir('.', filter = 'phonemes', invert = TRUE, stop_on_failure = TRUE)"
+


Suggested change

stray whitespace

Removed in 520b592

drammock · 2026-08-22T20:00:00Z

+test_that("multiple clicks in sequence with a modifier are all preserved", {
+    ## two different clicks + one modifier, all typed "M".
+    result <- order_ipa("ǀǁʰ")
+    expect(stri_detect_fixed(result, "ǀ"),
+           paste("First click character was lost from", shQuote(result)))
+    expect(stri_detect_fixed(result, "ǁ"),
+           paste("Second click character was lost from", shQuote(result)))
+    expect(nchar(result) == 3,
+           paste("Expected a 3-character result, got", shQuote(result),
+                 "(", nchar(result), "characters)"))
+})
+


Suggested change

test_that("multiple clicks in sequence with a modifier are all preserved", {

## two different clicks + one modifier, all typed "M".

result <- order_ipa("ǀǁʰ")

expect(stri_detect_fixed(result, "ǀ"),

paste("First click character was lost from", shQuote(result)))

expect(stri_detect_fixed(result, "ǁ"),

paste("Second click character was lost from", shQuote(result)))

expect(nchar(result) == 3,

paste("Expected a 3-character result, got", shQuote(result),

"(", nchar(result), "characters)"))

})

no point to test this as you can't have 2 click glyphs in a row (click glyphs must always be accompanied by a preceding base glyph that denotes the place of back closure; the click glyph denotes front closure). In other words ǀǁʰ would really have to be kǀkǁʰ or qǀkǁʰ or something like that

Removed test 9af5531

drammock · 2026-08-22T20:02:26Z

+test_that("two separate click+diacritic pairs in one string are both skipped", {
+    ## typestring "MDMD" both with clicks, so both should be skipped
+    result <- order_ipa("ǀ̥ǁ̥")
+    expect(stri_detect_fixed(result, "ǀ"),
+           paste("First click character was lost from", shQuote(result)))
+    expect(stri_detect_fixed(result, "ǁ"),
+           paste("Second click character was lost from", shQuote(result)))
+    expect(nchar(result) == 4,
+           paste("Expected a 4-character result, got", shQuote(result),
+                 "(", nchar(result), "characters)"))
+})
+


I think we can skip this one too, for the same reason as above

Suggested change

test_that("two separate click+diacritic pairs in one string are both skipped", {

## typestring "MDMD" both with clicks, so both should be skipped

result <- order_ipa("ǀ̥ǁ̥")

expect(stri_detect_fixed(result, "ǀ"),

paste("First click character was lost from", shQuote(result)))

expect(stri_detect_fixed(result, "ǁ"),

paste("Second click character was lost from", shQuote(result)))

expect(nchar(result) == 4,

paste("Expected a 4-character result, got", shQuote(result),

"(", nchar(result), "characters)"))

})

Removed test 9af5531

drammock · 2026-08-22T20:10:43Z

+    ## every add-features.R `special_cases` pair has a matching row in special-feature-table.csv 
+    special_cases <- c("pɸ", "pf", "tθ", "ts", "tʃ", "ʈʂ", "cç", "kx",
+                       "qχ", "bβ", "bv", "dð", "dz", "dʒ", "ɖʐ", "ɟʝ", "ɡɣ",
+                       "ɢʁ", "kp", "ɡb")
+    special_feats <- read.csv(file.path("..", "raw-data", "FEATURES",
+                              "special-feature-table.csv"),
+                              fileEncoding="UTF-8")
+    missing <- setdiff(special_cases, special_feats$segment)
+    expect(length(missing) == 0,
+           paste("special_cases pair(s) missing from special-feature-table.csv:",
+                 paste(missing, collapse=", ")))
+})


this is OK, but it seems like we could achieve the same thing by calling stopifnot(nrow(base_vec) == 1) right after the lookup, to make sure we got back exactly one row:

dev/scripts/add-features.R

Lines 145 to 146 in 6da1f9e

## look up the combined base glyphs in the special_feats table

base_vec <- special_feats[special_feats$segment == base_pair, ]

I agree, that is better. Avoids issue of hardcoded special_cases list in test.

Removed the test in f1c78ca
And renamed file to test-regression-c-cedilla.R since it only tests for c-cedilla now 792ea5a

Added the stopifnow assert in 9f8864d

Reran pipeline, phoible.csv has no diff with committed version.

…paces

…res.R

varunghat added 14 commits August 20, 2026 12:08

Added regression test for click-modifier, updated pixi.toml with task…

14a0fe2

…s and add-features

Added regression test for special-cases and c-cedilla

66016df

Excluded test-phonemes.R from ci, added output as pr comment

776e548

Fixed typo and test commands in workflows

21b0eb5

changed working directory of pr comment job

d8d96b9

Added comment about workflow change

bb31120

Added comment to test workflow

6add4c2

Testing edit-last in gh pr comment

9d695d8

reverted edit-last in gh pr comment

f455228

Added test to verify glottocode-isocode mapping matches glottolog

7a34af4

Added test to check inventory-bibtex mapping, InventoryID duplicates

1e6d451

Moved feature test to beginning so we get pr comment even if other te…

f8d0853

…sts fail

Moved workflow permissions to inside jobs

9e1a2e4

Split PR comment workflow to allow for PR from forked repos

42b863c

varunghat marked this pull request as draft August 20, 2026 23:27

varunghat marked this pull request as ready for review August 21, 2026 12:43

drammock requested changes Aug 22, 2026

View reviewed changes

varunghat added 5 commits August 23, 2026 14:15

Removed write permissions from main.yml actions, removed stray whites…

520b592

…paces

Removed two unnecessary tests from click-modifier regression test

9af5531

Changed special-case regression test to only c-cedilla

f1c78ca

Renamed test file suffix from special-casess to c-cedilla

792ea5a

Added check special_cases lookup returns exactly one row in add-featu…

9f8864d

…res.R

Navigation Menu

Uh oh!

More tests, workflows PR comment for feature test - #405

More tests, workflows PR comment for feature test#405
varunghat wants to merge 19 commits into
phoible:masterfrom
varunghat:feat/more-tests

varunghat commented Aug 20, 2026

Uh oh!

varunghat commented Aug 21, 2026

Uh oh!

Uh oh!

drammock Aug 22, 2026

Uh oh!

varunghat Aug 23, 2026

Uh oh!

drammock Aug 22, 2026

Uh oh!

varunghat Aug 23, 2026

Uh oh!

Uh oh!

drammock Aug 22, 2026

Uh oh!

varunghat Aug 23, 2026 •

edited

Loading

Uh oh!

drammock Aug 22, 2026

Uh oh!

varunghat Aug 23, 2026 •

edited

Loading

Uh oh!

drammock Aug 22, 2026

Uh oh!

varunghat Aug 23, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

	## look up the combined base glyphs in the special_feats table
	base_vec <- special_feats[special_feats$segment == base_pair, ]

Uh oh!

Conversation

varunghat commented Aug 20, 2026

Uh oh!

varunghat commented Aug 21, 2026

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

varunghat Aug 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

varunghat Aug 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

varunghat Aug 23, 2026 •

edited

Loading

varunghat Aug 23, 2026 •

edited

Loading