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

Fix workflow failing due to missing language tag #5379

Merged
merged 1 commit into from
Jan 13, 2025

Conversation

deining
Copy link

@deining deining commented Jan 8, 2025

With HEAD of repo (branch develop), the workflow run of GitHub action static-analysis currently fails. This PR fixes that issue.

Checklist:

  • Workflown run of action 'static-analysis' is now successful

@andydotxyz
Copy link
Member

Can you please clarify which failure this was addressing?

@coveralls
Copy link

coveralls commented Jan 8, 2025

Coverage Status

coverage: 59.204% (-0.04%) from 59.244%
when pulling b0cc199 on deining:fix-static-analysis
into 36de697 on fyne-io:develop.

@Jacalz
Copy link
Member

Jacalz commented Jan 8, 2025

I would also appreciate if the title was clearer. Something like: "Fixed XYZ causing static analysis to fail" or similar. Makes it a bit clearer when looking at the PR list

@deining deining force-pushed the fix-static-analysis branch from 639d2ea to 152b0ac Compare January 12, 2025 20:43
@deining deining changed the title Action workflow 'static analysis': fix error Setting LANG, absence caused action workflow 'static analysis' to fail Jan 12, 2025
@deining
Copy link
Author

deining commented Jan 12, 2025

Can you please clarify which failure this was addressing?

Sorry for not being clear here, I will elaborate a bit more on my PR and its intention:

Actual:

Currently, 4a30df3 is the latest commit on branch develop. As seen form this log, workflow static-analysis fails for this commit. This the error message:

fyne.io/fyne/v2/internal/scale		coverage: 0.0% of statements
	fyne.io/fyne/v2/internal/theme		coverage: 0.0% of statements
2025/01/12 14:41:47 Fyne error:  Error parsing user locale C
2025/01/12 14:41:47   Cause: language: tag is not well-formed
2025/01/12 14:41:47   At: /home/runner/work/fyne/fyne/lang/locale.go:35

The error is due to the fact that with a fresh ubuntu installation, there are no locales present other than locale C:

$ locale -a
C
C.utf8

My PR:

My PR adds two line to the workfile file static_analysis: the first line enforces installation of package language-pack-en. After installation of this package, several more locales are now present:

$ locale -a
C
C.utf8
en_AG
en_AG.utf8
en_AU.utf8
en_BW.utf8
en_CA.utf8
en_DK.utf8
en_GB.utf8
en_HK.utf8
en_IE.utf8
en_IL
en_IL.utf8
en_IN
en_IN.utf8
en_NG
en_NG.utf8
en_NZ.utf8
en_PH.utf8
en_SG.utf8
en_US.utf8
en_ZA.utf8
en_ZM
en_ZM.utf8
en_ZW.utf8

The second line sets and exports the environment value LANG to the value en_EN.UTF-8:

export LANG=en_EN.UTF-8

After adding these two lines, the workflow run is now successful:

https://github.com/fyne-io/fyne/actions/runs/12736939645

I hope this makes the intention of this PR clear.

@Jacalz Jacalz changed the title Setting LANG, absence caused action workflow 'static analysis' to fail Fix workflow failing due to missing laguage tag Jan 12, 2025
@Jacalz
Copy link
Member

Jacalz commented Jan 12, 2025

Thanks for the explanation. That sounds like a useful fix but I don't think it only applies to the static analysis test. I've seen it in the platform tests as well, I think.

I wonder why this isn't happening all of the time? The static analysis test certainly does succeed most of the time but occasionally behaves like this.

@deining deining changed the title Fix workflow failing due to missing laguage tag Fix workflow failing due to missing language tag Jan 12, 2025
@deining deining mentioned this pull request Jan 12, 2025
1 task
@deining
Copy link
Author

deining commented Jan 12, 2025

Thanks for the explanation.

You are welcome.

That sounds like a useful fix

Glad to hear that.

but I don't think it only applies to the static analysis test. I've seen it in the platform tests as well, I think.

You are completely right, this issue is also present with the platform tests. To address this issue, I authored commit a5cf0fc which is part of my PR #5367.

I wonder why this isn't happening all of the time?

Sorry, I have no idea here.

@andydotxyz
Copy link
Member

Just to be clear, this is not fixing any actual test failures or runner failures.
There must be some case that is handling the failure and returning.

So the impact is probably that more code is being executed by the teats - but develop is green on all runners at the moment.

@andydotxyz
Copy link
Member

The "Fyne Error" output in a test or a console is a printout of an internal issue. It does not (normally?) denote a test failure.

@deining
Copy link
Author

deining commented Jan 13, 2025

Just to be clear, this is not fixing any actual test failures or runner failures.

Yes, that is very true.

So the impact is probably that more code is being executed by the tests

Yes, that's correct.

but develop is green on all runners at the moment.

  • for the main branch: yes
  • for the develop branch: no

This PR targets at getting the checkers green for the develop branch, too.

@andydotxyz
Copy link
Member

but develop is green on all runners at the moment.

  • for the main branch: yes
  • for the develop branch: no

This PR targets at getting the checkers green for the develop branch, too.

This is yesterday's develop run:

Screenshot 2025-01-13 at 10 28 32

@deining
Copy link
Author

deining commented Jan 13, 2025

This is yesterday's develop run:

There are four more commits since commit 0691093 mentioned by you:

grafik

Two of these commits have their checker green, two show red as result. I would like to see 4 green results here.

@Jacalz
Copy link
Member

Jacalz commented Jan 13, 2025

The failure in https://github.com/fyne-io/fyne/actions/runs/12745616918/job/35519942661 for example, i.e. the latest commit on develop as of now, does print the message that you are de scribing but it is not the cause for the failure. Go tests only print their stdout if something fails. The real issue is a flaky test in the Glfw driver testing.

@andydotxyz
Copy link
Member

Two of these commits have their checker green, two show red as result. I would like to see 4 green results here.

Agreed, but as discussed above this PR does not actually impact the red/green status. It does, however improve the test effectiveness.
You can see that because there is a (same) red cross on this PR as well...

@deining deining force-pushed the fix-static-analysis branch 2 times, most recently from b73cba2 to 30029d6 Compare January 13, 2025 12:16
@deining deining force-pushed the fix-static-analysis branch from 30029d6 to b0cc199 Compare January 13, 2025 12:24
@deining
Copy link
Author

deining commented Jan 13, 2025

Two of these commits have their checker green, two show red as result. I would like to see 4 green results here.

Agreed, but as discussed above this PR does not actually impact the red/green status. ...
You can see that because there is a (same) red cross on this PR as well...

I improved my PR, now we do have a green checker for this PR 😄.
Unfortunately, that's not the end of the story. As @Jacalz correctly pointed out, there is still a flaky test in the Glfw driver testing.

@andydotxyz
Copy link
Member

As @Jacalz correctly #5379 (comment), there is still a flaky test in the Glfw driver testing.

Yes indeed there is a flakey test which is making the red marks on the tests occasionally.

For future reference, you can see the cause of a test failure using the standard go test output --- FAIL: whereas the Fyne error: is just logging of an internal error and won't fail the build.

@andydotxyz andydotxyz merged commit 40c2a56 into fyne-io:develop Jan 13, 2025
12 checks passed
@andydotxyz
Copy link
Member

Thanks for this enhancement, and congratulations on landing your first Fyne PR :)

@deining deining deleted the fix-static-analysis branch January 13, 2025 15:58
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.

4 participants