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.

Sign up for GitHub

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

Jump to bottom

CI failure in git.commitEntry test #1803

Closed
ST-DDT opened this issue Feb 1, 2023 · 18 comments · Fixed by #1813
Closed

CI failure in git.commitEntry test #1803

ST-DDT opened this issue Feb 1, 2023 · 18 comments · Fixed by #1813
Labels
c: bug Something isn't working c: test good first issue Good for newcomers p: 2-high Fix main branch s: accepted Accepted feature / Confirmed bug
Milestone
v8.x

Comments

@ST-DDT
Copy link
Member

ST-DDT commented Feb 1, 2023 •
edited
Loading

VITEST_SEQUENCE_SEED 1675266128640

grafik

https://github.com/faker-js/faker/actions/runs/4066068064/jobs/7002435580

The regex needs to be checked for correctness (blindy adding ' is a non goal).
@ST-DDT ST-DDT added c: bug Something isn't working p: 2-high Fix main branch c: test s: accepted Accepted feature / Confirmed bug labels Feb 1, 2023
@ST-DDT ST-DDT moved this to Todo in Faker Roadmap Feb 1, 2023
@ST-DDT
Copy link
Member Author

ST-DDT commented Feb 1, 2023

Also:

VITEST_SEQUENCE_SEED 1675271531940
grafik
https://github.com/faker-js/faker/actions/runs/4066878355/jobs/7003536672

@ST-DDT ST-DDT added the good first issue Good for newcomers label Feb 1, 2023
@ST-DDT
Copy link
Member Author

ST-DDT commented Feb 1, 2023

The same error again:

VITEST_SEQUENCE_SEED 1675275945101
grafik

https://github.com/faker-js/faker/actions/runs/4067493397/jobs/7004904658

@matthewmayer
Copy link
Contributor

the name comes from faker.person.fullName, which in the English locale can contain ' in addition to A-Za-z.

Whereas the regex only allows \w (which doesn't include '), . and space.

So blindly adding ' would indeed fix it.

If the same test was run in other locales, it would still fail as they could contain other unicode chars. But as the test is only run in English at the moment i think this would suffice.

@matthewmayer
Copy link
Contributor

btw is there any way in the logs to make it print out the full string for expected value? The way it ellipsizes the full value is annoying.

@matthewmayer matthewmayer mentioned this issue Feb 2, 2023
Closed
@Shinigami92
Copy link
Member

Shinigami92 commented Feb 2, 2023 •
edited
Loading

btw is there any way in the logs to make it print out the full string for expected value? The way it ellipsizes the full value is annoying.

Vitest also has a documentation website, even with search function
https://vitest.dev/config/#outputtruncatelength

Now the funny part: the option does not work 🤦
I will open an issue in Vitest...

@ST-DDT
Copy link
Member Author

ST-DDT commented Feb 2, 2023

So blindly adding ' would indeed fix it.

I would still like to know which characters are allowed there (and how it is called).

@Shinigami92
Copy link
Member

vitest-dev/vitest#2791

@xDivisionByZerox
Copy link
Member

We could see the failing test as an implementation error as well.
The questions we have to answer are:

  • Which characters are actually allowed to be included in the author line?
  • Which characters do we allow in the author line?

If the same test was run in other locales, it would still fail as they could contain other unicode chars.

This could be solved by defining that the generated user is always a username instead of using a fullname.

@matthewmayer
Copy link
Contributor

Unicode characters are permissible according to https://stackoverflow.com/questions/24845258/is-it-ok-to-use-a-long-non-ascii-name-for-the-user-name-git-configuration

@matthewmayer
Copy link
Contributor

Honestly I'd just add the ' for now to fix the intermittent CI errors and leave a TODO to revisit this later if tests are added for every locale. Regex for Unicode text can be quite tricky, and I don't think this is a heavily used method in faker.

@Shinigami92
Copy link
Member

vite repo `git log --pretty=format:%an | sort -u` 👀 
${Mr.DJA}
07akioni
0xflotus
4quar1usnow
Aaron Bassett
Aaron Cordova
Aaron Gong
Aayush Kapoor
Abdellah Alaoui Solaimani
Agustin Gomez
Ahab
Akito Tabira
Alec Larson
Alex Kozack
Alex Lambiris
Alexander Belov
Alexander Raihelgaus
Alexander von Weiss
Alexandre
Alexandre Bonaventure Geissmann
Alexandre Hitchcox
Alexey Shamrin
Alexis Tyler
Aleš Vaupotič
Amit Gurbani
Amorites
Amour1688
Anbraten
Andoni Abedul
Andreas Girgensohn
Andrejs Abrickis
Andrew
Andrew Powers
Andrew Welch
Andrey Trebler
Andy Li
Angel Gomez
Ant
Ante Sepic
Anthony Campolo
Anthony Catel
Anthony Fu
Arnaud Barré
Arnaud Thomas D
Aron Griffis
Artem Khodyush
Artur
Ash Go
Ashish Shubham
Asko Kauppi
Autumn Meadow
Axe
Azat S
Bart Louwers
Barthélémy Ledoux
Bartosz Gościński
Bechir Ba
Ben
Ben Halpern
Ben Holmes
Ben McCann
Benedikt Allendorf
BenoitRanque
Bernardo Sunderhus
Berton Zhu
Bjorn Lu
Blake Newman
Bogdan Chadkin
Borkesz Máté
Bozhen Zheng
Brett Pappas
Bri
Brice BERNARD
Bálint Szekeres
CGQAQ
CHOYSEN
CRIMX
CSY54
Caleb Eby
Candy
Carlos Morales
Carlos Rodrigues
Carter Snook
Cassio Zen
Cedric Nicoloso
Cesar Gomez
Charles C
Chris
Chris Santamaria
Christoph Nakazawa
Christoph Werner
Christopher Shank
Claire
Clay
Clement Fradet Normand
Cláudio Medina
Costa Alexoglou
Cristian Pallarés
Curran Kelleher
Cyrille Bourgois
Cédric Eberhardt
Daiki Ojima
Daiz
Dalibor Gogic
Damian Stasik
Dan
Dan Jutan
Daniel Imfeld
Daniel Roe
Daniel Schmelz
Danny Feliz
Dany Castillo
Darin Cassler
Darren Jennings
David Bohn
David Bonnet
David Jackson
David R. Myers
David Öhlin
Day
Deisling Eduard
Dimitris-Toulis
Domantas
Dominik G
DouglasDev
Drew Powers
Dunqing
Dustin Ryerson
Dylan Piercey
EGOIST
Eder Soares
Eduardo San Martin Morote
Edward Sanders
Eirik Sletteberg
Em Zhan
Enzo Innocenzi
Eren Bets
Eric Eastwood
Eric Jolibois
Espen Hovlandsdal
Etienne
Eugene Kopich
Eugene Kopich ⚡
Evan Boehs
Evan You
Fansy
Faris Ansari
Fatih Aygün
Ferenc Tamás
Fernanda Sales Bittencourt de Lemos
Florian Dreier
Flyme
Forzen FIsh
Fran Dios
Francois Valdy
François Risoud
François Wouts
Fred K. Schott
GaryNg
Gautier Ben Aïm
GavinRay97
Georgiy Bukharov
Glen Huang
Glen Little
Gray Zhang
Greg Leppert
GrygrFlzr
Guanghui Cheng
Guille
Guillermo Rauch
HChange
HG
Hang Liang
Han（ハン）
Haoqun Jiang
Happydev
Harry Brundage
Harry Porter-Mills
Harsh Pandey
Haruki Murasaki
Hazlank
HcySunYang
HelKyle
Henry
Henry Vogt
Hermann Stanew
Herrington Darkholme
Hex
Homyee King
Hugo ATTAL
Hugo Alliaume
Hung Viet Nguyen
Hydrogen
IU
Ian Purvis
Ian VanSchooten
Ian Wright
Ilya Artamonov
Iron Lu
Isaac Levy
Israel Roldan
Ivan Demchuk
Ivan Sieder
J Bruni
JA
Jack Chu
Jack Franklin
Jambon
James Garbutt
James George
James Herdman
James Lee
James Tucker
Jamie Spittal
Janis Pritzkau
Jasper Van Gestel
Jay
JayFate
Jeff Yang
Jeff Yang (Nay Thu Ya Aung)
Jen Looper
Jesse
Jessica Sachs
Jeudi Prando Araujo
Jian Zhang
Jiby Jose
Joaquín Sánchez
Joe Busillo
Joel Ellis
Joel Kuzmarski
Johan Bergström
Johannes
Johannes Maas
Johannes Schickling
John Simons
Johnson Chu
JokcyLou
Jonas
Jonas Galvez
Jonathan Herman
Jordan Pittman
Josef Brandl
Josh Field
Josh Larson
Joshua Davidson
JounQin
Jovi De Croock
JserWang
Juan Martín Seery
Julien Papini
Jun Shindo
Junwen Huang
Justin
Jérémie Cotiniaux
Kael
Kallyn Gowdy
Kazuma Oe
Kenichi Kamiya
Kerim Hudson
Kevin Hazy
Kevin LEVRON
Kia King Ishii
Kid
KonpekiCode
Kristoffer K
Kyle Herock
Lee
Lee Robinson
Li Chen
Li Zhequ
Lijianan
Lil-C0der
Liu Bowen
LocTran016
Lubomír Blažek
Lucas Garron
Lucas Mendelowski
Ludovico Fischer
Lukas
Luuk de Vlieger
MaMrEzO
Maarten de Graaf
Manan Tank
Manu MA
Marcel Lindig
Marco Schumacher
Mark
Mark Ladyshau
Markus Lanthaler
Martin Kirilov
Marvin Hagemeister
Marvin Rudolph
Mat
Mathieu Mure
Matia
Matias
Matias Capeletto
Matt Nathan
Matthew Phillips
Matthew Runyon
Matěj Volf
Maurício Kishi
Meet Zaveri
Menci
Mestery
Michael Kurze
Michael Oliver
Michael P Smith
Michael Warner
Michel EDIGHOFFER
Mike Ledger
Minseok Suh
Modder Me
Máximo Mussini
NONAME
Naeemo
Naitik Shah
Naoki Endoh
Nate Moore
Natrim
Nick Santos
Nicolas Hedger
Nihal Gonsalves
Niputi
Nurettin Kaya
Nurul Huda (Apon)
OBE Rocks!
Obinna Ekwuno
Oliver Zhou  (毓杰)
OliverTsang
Olyno
OmegaVesko
OneNail
Ophir LOJKINE
Oreki S.H
Ostap B
Oyster Lee
Patrick Dobbs
Paulius Varna
Pavel Zhukov
Pedro Di Martino
Peng Xiao
Pengsha Ying
Percy Ma
Pete Nykänen
Peter Chibisov
Peter Mekhaeil
Phil Chen
Phinome
PiEgg
Pieter
Pig Fang
Pony Yan
Prana Adiwira
QiChang Li
Rahul Kadyan
Ramnath Shenoy
Razio
Razvan Stoenescu
Reed Jones
Remus Mate
Renovate Bot
Rich Harris
Richard Petersen
Rivaldo Junior
Robin Hellemans
Rody Davis
Rom
Rom Brillout
Roman
Roman Imankulov
Romuald Brillout
Rongjian Zhang
Ruicong
Ryan Christian
Ryan Moyo
Ryan Tsao
Ryan Wang
Rémi Emonet
Sacha STAFYNIAK
Sachin Raja
Sam
Sam Richard
Sam Verschueren
Samuel Alev
Saurabh Daware
Sean Deng
Sean Watters
Sebastian Krüger
Sebastian Seilund
SegaraRai
Segev Finer
Sepehr Safari
Sepush
Sergey Vinogradov
SeyyedKhandon
Shahriar Shojib
Sharvilak
Shigma
Shinigami
Shinigami92
Shyam Chen
Shyim
Simon
Simon H
Simon Legner
Simon · 西萌
Sirui Chen
Sleepy Five
Sociosarbis
Songkeys
Sosuke Suzuki
Stefan van Herwijnen
Steve Lee
Steve Shreeve
Steven T
Steven Waters
Sting Alleman
Sunil Pai
Surmon
Sven
Svilen Ivanov
Swedish-li
TJ Koblentz
TMQ
Takashi MIMA
Tal500
Tan Li Hau
Tedy
Thai Pangsakulyanont
Theo Browne
Thorsten Lünborg
Tiep Nguyen
Tiger Oakes
Tim MacDonald
Timo Zander
Timsonrobl
Titouan Mathis
Tmk
Tobias Lundin
Tobias Melén
Tobias Nießen
Tomas Beres
Tomasz Surowiec
Tommaso De Rossi
Tomáš Livora
Tony Schaffert
Tony Trinh
Touchumind
TrickyPi
TropicalRaisel
Troy Latchman
Turmiht
Tyler Rockwood
Umidbek Karimov
Valentin
Vben
Victor
Victor Eke
Victor Trigo Wagner
Vitaly Baev
Vladimir
Vojtech Miksu
WORMSS
Wang Jian
Wang Yifei
Wei
Wenlu Wang
Wes Harper
William
Xavier Cambar
Xin Du (Clark)
Xuguang Mei
Yadhunandan S
Yan Savinov
Yanlin Jiang
Yaroslav Dobzhanskij
Yash Singh
Yehyoung Kang
Yelmor
Yiyang Sun
Yoshi Togami
Yunfei He
Yunwei Xiao
ZHAO Jinxiang
ZYSzys
Zahin Afsar
Zehua Chen
Zetao Zhuang
Zhang Zhi
Zhao
Zheeeng
ZhengX
Zoran Pesic
ZuoChenxue
ZxBing0066
abderrahmen lahmedi
agoni1212
alex
alykamb
amandeep-r
andrew0
andyjxli
ashish
bbbboom
bbotto-pdga
bompus
btea
bz-pepite
casret
chaxus
chencheng (云谦)
chenjiajian
chlorine
chris-zhu
circle-gon
cisen
clocher Zhong
codeoD
codpoe
csr632
cyitao
davidbielik
dayuan
dengqing
dependabot[bot]
dragonHandsome
edison
enya
falstack
ferdinando-ferreira
fi3ework
finico
fishDog
ghostdevv
gnehc
haiya6
hardfist
heart
hehe
hemengke
hiro
hiro-lapis
hiroki
hisland
hongmin.qiao
hyrious
idler
ikeq
imShara
itibbers
j000
jbmolle
jods
jsyanyang
kakachake
kamilic
kazuya kawaguchi
krystal
kuake
leemove
li.li
libing
lijialiang
lijianzhang
likui
liuwei
lsdsjy
ltaoo
luke miles
meijin
meteorlxy
metonym
milahu
mj2ks
mori yuta
moyinzi
mstoller-centricity
musicq
mwyywm
neal
nio89
ocavue
odanado
okxiaoliang4
oliverpool
patak
patak-dev
patak-js
pengbo
plq
pooya parsa
poyoho
psaren
qicoo
qmhc
rainkolwa
reid j sherman
renovate[bot]
rhan
rixo
roger6106
rs3d
rxliuli
sagargurtu
sanyuan
sapphi-red
shengxinjing
shir0u
shiyu
shj
sibbng
simohamed
smeng9
smnhgn
spencer17x
ssh
stefnotch
stygian-desolator
sun0day
swandir
taisei mima
tessaSAC
tianyu94
toSayNothing
toshify
tuchg
underfin
untp
vaakian X
valley
vemoo
virgosoy
vwkd
wang
wangkaiwd
webfansplz
wmzy
wxy
xiejiahe
xingxiuyi
xrkffgg
xyl66
yArna
yangxingyuan
ygj6
yinzhenyu
yoho
ysy945
yuangongji
yue
yuuang
yuz
zdw
zhangenming
zhangrenyang
zhenzhenChange
ziga
zk4
zoomdong
zygzagZ
zz
İsmail Arılık
ᴜɴвʏтᴇ
⚡ Aditya Patel ⚡
三咲智子
三毛
云游君
似水微寒
兰天游
刘巍峰
宇cccc
宋来瑞
宋铄运 (Alan Song)
弘树@阿里
思翊
文蔺
李墨
杨兆雨
杨永安
沈青川
沧浪
清尘
王凯
粒粒橙
翠 / green
耗子
胡金鑫
花开半亩地
草鞋没号
蜗牛老湿
遊真uZ
那里好脏不可以
马鹏达
Ｂｅｎ Ｔｅａ

@matthewmayer
Copy link
Contributor

instead of [\w_\. ']+ you could just do something like [^<]+ as < will be the first char of the email address.

i noticed if i do
git config user.name "matt < hello"
then my author line looks like
Author: matt hello <[email protected]>

so it seems to be stripping out angle brackets to avoid confusion with the email

@matthewmayer
Copy link
Contributor

bit more info on stripping:

libgit2/libgit2#5342

@Shinigami92
Copy link
Member

Now we coming closer to an appropriated solution 👍

@xDivisionByZerox
Copy link
Member

So we should actually refactor the implementation to normalize the username/full name before using it in the commit entry.

@ST-DDT
Copy link
Member Author

ST-DDT commented Feb 2, 2023

What are you referring to with normalization? IMO we should make sure we pass the correct validation, but for that we need to actually have the correct validation.

@Shinigami92
Copy link
Member

Shinigami92 commented Feb 2, 2023 •
edited
Loading

What are you referring to with normalization? IMO we should make sure we pass the correct validation, but for that we need to actually have the correct validation.

They mean normalize the generated values inside commitEntry to strip out invalid chars

faker/src/modules/git/index.ts

Lines 108 to 122 in 1ae2f6f

const firstName = this.faker.person.firstName();
const lastName = this.faker.person.lastName();
const fullName = this.faker.person.fullName({ firstName, lastName });
const username = this.faker.internet.userName(firstName, lastName);
const user = this.faker.helpers.arrayElement([fullName, username]);
const email = this.faker.internet.email(firstName, lastName);
lines.push(
`Author: ${user} <${email}>`,
`Date: ${this.commitDate({ refDate })}`,
'',
`\xa0\xa0\xa0\xa0${this.commitMessage()}`,
// to end with a eol char
''
);

Like doing an extra step in line 114 for variable user

@matthewmayer
Copy link
Contributor

Hmm how would you test that though considering fullName never includes a <

@Shinigami92 Shinigami92 mentioned this issue Feb 3, 2023
Merged
@github-project-automation github-project-automation bot moved this from Todo to Done in Faker Roadmap Feb 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
c: bug Something isn't working c: test good first issue Good for newcomers p: 2-high Fix main branch s: accepted Accepted feature / Confirmed bug
Projects
No open projects
Faker Roadmap
Status: Done
Milestone
v8.x
4 participants
@matthewmayer @ST-DDT @Shinigami92 @xDivisionByZerox