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: unit tests fail in windows #1003

Merged
merged 2 commits into from
Nov 29, 2023
Merged

Conversation

Skyenought
Copy link
Contributor

@Skyenought Skyenought commented Nov 18, 2023

What type of PR is this?

fix

Check the PR title.

  • This PR title match the format: <type>(optional scope): <description>
  • The description of this PR title is user-oriented and clear enough for others to understand.
  • Attach the PR updating the user documentation if the current PR requires user awareness at the usage level. User docs repo

(Optional) Translate the PR title into Chinese.

(Optional) More detailed description for this PR(en: English/zh: Chinese).

en:
zh(optional): 修复单元测试在 Windows 环境下失败的问题

在 windows 环境下, 文件路径一般形为 E:/gopath/f.go, 不为 unix 的 /gopath/f.go
该函数并没有考虑到 windows 的这种情况

// Maybe rawURL is of the form scheme:path.
// (Scheme must be [a-zA-Z][a-zA-Z0-9+-.]*)
// If so, return scheme, path; else return nil, rawURL.
func getScheme(rawURL []byte) (scheme, path []byte) {
	for i := 0; i < len(rawURL); i++ {
		c := rawURL[i]
		switch {
		case 'a' <= c && c <= 'z' || 'A' <= c && c <= 'Z':
		// do nothing
		case '0' <= c && c <= '9' || c == '+' || c == '-' || c == '.':
			if i == 0 {
				return nil, rawURL
			}
		case c == ':':
			if i == 0 {
				hlog.Errorf("error happened when try to parse the rawURL(%s): missing protocol scheme", rawURL)
				return nil, nil
			}
			return rawURL[:i], rawURL[i+1:]
		default:
			// we have encountered an invalid character,
			// so there is no valid scheme
			return nil, rawURL
		}
	}
	return nil, rawURL
}


#### (Optional) Which issue(s) this PR fixes:
<!--
Automatically closes linked issue when PR is merged.
Eg: `Fixes #<issue number>`, or `Fixes (paste link of issue)`.
-->

#### (Optional) The PR that updates user documentation:
<!--
If the current PR requires user awareness at the usage level, please submit a PR to update user docs. [User docs repo](https://github.com/cloudwego/cloudwego.github.io)
-->

@Skyenought Skyenought requested review from a team as code owners November 18, 2023 03:22
@Skyenought Skyenought force-pushed the fixwin branch 3 times, most recently from 21fb725 to 71c2c01 Compare November 18, 2023 07:21
Copy link

codecov bot commented Nov 20, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (89e9721) 82.52% compared to head (293ae3c) 82.55%.

Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #1003      +/-   ##
===========================================
+ Coverage    82.52%   82.55%   +0.03%     
===========================================
  Files           98       98              
  Lines         9962     9964       +2     
===========================================
+ Hits          8221     8226       +5     
+ Misses        1244     1242       -2     
+ Partials       497      496       -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@FGYFFFF
Copy link
Contributor

FGYFFFF commented Nov 21, 2023

@Skyenought 这个在 windows 下的哪个 case 会有问题?

@li-jin-gou
Copy link
Member

li-jin-gou commented Nov 21, 2023

@Skyenought 这个在 windows 下的哪个 case 会有问题?

看下单测 https://github.com/cloudwego/hertz/actions/runs/6912348902/job/18823241892?pr=1004

@Skyenought
Copy link
Contributor Author

@FGYFFFF 所有使用 router.StaticFile 的 case

hertz/pkg/protocol/uri.go

Lines 405 to 417 in 9c3f0b7

func (u *URI) parse(host, uri []byte, isTLS bool) {
u.Reset()
if stringContainsCTLByte(uri) {
return
}
if len(host) == 0 || bytes.Contains(uri, bytestr.StrColonSlashSlash) {
scheme, newHost, newURI := splitHostURI(host, uri)
u.scheme = append(u.scheme, scheme...)
bytesconv.LowercaseBytes(u.scheme)
host = newHost
uri = newURI

因为在单侧满足 if len(host) == 0 || bytes.Contains(uri, bytestr.StrColonSlashSlash) 的 len(host) == 0 条件
在实际使用的时候是不会满足这个 case 的

@Skyenought Skyenought force-pushed the fixwin branch 2 times, most recently from 40712fb to 6a2943e Compare November 22, 2023 03:18
@welkeyever welkeyever changed the title fix unit test fail in windows env fix: unit tests fail in windows Nov 22, 2023
pkg/protocol/uri.go Outdated Show resolved Hide resolved
@welkeyever
Copy link
Member

Good job! There has a few todos before merging.

@Skyenought Skyenought force-pushed the fixwin branch 2 times, most recently from 3dee6ef to 4357aad Compare November 23, 2023 23:59
@Skyenought Skyenought force-pushed the fixwin branch 2 times, most recently from ee0a562 to 5723283 Compare November 24, 2023 07:44
@Skyenought Skyenought force-pushed the fixwin branch 2 times, most recently from 610be1e to f75f3b0 Compare November 25, 2023 06:39
@Skyenought Skyenought force-pushed the fixwin branch 7 times, most recently from 2450447 to 82114e8 Compare November 27, 2023 05:27
Copy link
Member

@welkeyever welkeyever left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thx~

@welkeyever
Copy link
Member

cc @cloudwego/hertz-reviewers

@li-jin-gou li-jin-gou merged commit a327795 into cloudwego:develop Nov 29, 2023
16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants