Skip to content

Commit

Permalink
Enhancements of tests diff
Browse files Browse the repository at this point in the history
This implements For SPRT:
verify that the base branch matches official-stockfish/master at the time of submission, or be able to see these differences in exceptional cases.
For SPSA:
Display the exact difference between the code that runs and official-stockfish/master at the time of submission
and fixes some bugs
  • Loading branch information
peregrineshahin authored and ppigazzini committed Apr 7, 2024
1 parent 505a0ab commit b5c9729
Show file tree
Hide file tree
Showing 7 changed files with 173 additions and 54 deletions.
4 changes: 4 additions & 0 deletions server/fishtest/rundb.py
Original file line number Diff line number Diff line change
Expand Up @@ -127,6 +127,8 @@ def new_run(
info="",
resolved_base="",
resolved_new="",
master_sha="",
official_master_sha="",
msg_base="",
msg_new="",
base_signature="",
Expand Down Expand Up @@ -163,6 +165,8 @@ def new_run(
"threads": threads,
"resolved_base": resolved_base,
"resolved_new": resolved_new,
"master_sha": master_sha,
"official_master_sha": official_master_sha,
"msg_base": msg_base,
"msg_new": msg_new,
"base_options": base_options,
Expand Down
2 changes: 2 additions & 0 deletions server/fishtest/schemas.py
Original file line number Diff line number Diff line change
Expand Up @@ -430,6 +430,8 @@ def final_results_must_match(run):
"threads": suint,
"resolved_base": sha,
"resolved_new": sha,
"master_sha": sha,
"official_master_sha": sha,
"msg_base": str,
"msg_new": str,
"base_options": str,
Expand Down
2 changes: 1 addition & 1 deletion server/fishtest/templates/base.mak
Original file line number Diff line number Diff line change
Expand Up @@ -354,7 +354,7 @@
target="_blank"
rel="noopener"
class="links-link rounded release"
>Prerelease</a
>Prereleases</a
>
</li>
<li>
Expand Down
190 changes: 137 additions & 53 deletions server/fishtest/templates/tests_view.mak
Original file line number Diff line number Diff line change
Expand Up @@ -99,6 +99,15 @@
Copy apply-diff command
</a>

<a
href="javascript:"
id="master_vs_official_master" class="btn btn-danger border-0 mb-2"
title="Compares master to official-master at the time of submission"
style="display: none">
<i class="fa-solid fa-triangle-exclamation"></i>
<span> master vs official</span>
</a>

<span class="text-success copied text-nowrap" style="display: none">Copied!</span>
</h4>
<pre id="diff-contents" style="display: none;"><code class="diff"></code></pre>
Expand Down Expand Up @@ -570,9 +579,11 @@

// Define some functions to fetch the diff and show it

function showDiff(text, numLines) {
const toggleBtn = document.getElementById("diff-toggle");
const diffContents = document.getElementById("diff-contents");
function addDiff (diffText, text) {
diffText.textContent = text;
}

function showDiff(diffContents, diffText, numLines) {
// Hide large diffs by default
if (numLines < 50) {
diffContents.style.display = "";
Expand All @@ -587,44 +598,25 @@
toggleBtn.textContent = "Show";
}, 350);
}
const diffText = diffContents.querySelector("code");
diffText.textContent = text;

// Try to save the diff in sessionStorage
// It can throw an exception if there is not enough space
try {
sessionStorage.setItem("${run['_id']}", text);
sessionStorage.setItem("${run['_id']}" + "_lines", numLines);
} catch (e) {
console.warn("Could not save diff in sessionStorage");
}

toggleBtn.addEventListener("click", function () {
diffContents.style.display =
diffContents.style.display === "none" ? "" : "none";
if (copyDiffBtn)
copyDiffBtn.style.display =
copyDiffBtn.style.display === "none" ? "" : "none";
if (toggleBtn.textContent === "Hide") toggleBtn.textContent = "Show";
else toggleBtn.textContent = "Hide";
});

document.getElementById("diff-section").style.display = "";
hljs.highlightElement(diffText);
}

async function getFileContentFromUrl(url) {
try {
const response = await fetch(url);
if (!response.ok) {
throw new Error(
"Failed to fetch " + url + ": " + response.status + " " + response.statusText
);
if (response.status === 404) {
return "";
} else {
throw new Error(
"Failed to fetch " + url + ": " + response.status + " " + response.statusText
);
}
}
return await response.text();
} catch (error) {
console.log("File may have been deleted, error fetching file from URL "+ url + ":", error);
return "";
throw new Error(
"Failed to fetch file: " + error
);
}
}

Expand All @@ -633,17 +625,15 @@
const url = apiUrl + "/git/trees/" + branch + "?recursive=1";
const response = await fetch(url);
if (!response.ok) {
throw new Error(
"Failed to fetch files in branch " + branch + ": " + response.status + " " + response.statusText
);
return null;
}
const data = await response.json();
return data.tree
.filter((entry) => entry.type === "blob")
.map((entry) => entry.path);
} catch (error) {
console.error("Error fetching files in branch " + branch + ": ", error);
return [];
return null;
}
}

Expand All @@ -661,22 +651,24 @@
}
}

async function fetchDiffTwoDots(rawContentUrl, apiUrl, diffBase, diffNew) {
async function fetchDiffTwoDots(rawContentUrlNew, rawContentUrlBase, apiUrlNew, apiUrlBase, diffNew, diffBase) {
try {
const [files1, files2] = await Promise.all([
getFilesInBranch(apiUrl, diffBase),
getFilesInBranch(apiUrl, diffNew),
getFilesInBranch(apiUrlBase, diffBase),
getFilesInBranch(apiUrlNew, diffNew),
]);

if (files1 === null || files2 === null)
return;
const allFiles = Array.from(new Set([...files1, ...files2]));
let diffs = await Promise.all(
allFiles.map(async (filePath) => {
const [content1, content2] = await Promise.all([
getFileContentFromUrl(
rawContentUrl + "/" + diffBase + "/" + filePath
rawContentUrlNew + "/" + diffBase + "/" + filePath
),
getFileContentFromUrl(
rawContentUrl + "/" + diffNew + "/" + filePath
rawContentUrlBase + "/" + diffNew + "/" + filePath
),
]);

Expand Down Expand Up @@ -724,34 +716,126 @@
"//api.github.com/repos/"
);

const rawContentUrl = "${h.tests_repo(run)}".replace(
let dots = 2;

const testRepo = "${h.tests_repo(run)}";
const rawContentUrlNew = testRepo.replace(
"//github.com/",
"//raw.githubusercontent.com/"
);
const apiUrl = "${h.tests_repo(run)}".replace(
const apiUrlNew = testRepo.replace(
"//github.com/",
"//api.github.com/repos/"
);
const diffBase = "${"master" if run["args"].get("spsa") else run["args"]["resolved_base"][:10]}";

const diffNew = "${run["args"]["resolved_new"][:10]}";
const rawOfficialMaster = "https://raw.githubusercontent.com/official-stockfish/Stockfish";
const apiOfficialMaster = "https://api.github.com/repos/official-stockfish/Stockfish";
const baseOfficialMaster = "${run["args"]["official_master_sha"][:10] if run["args"].get("official_master_sha") else ""}";

% if run["args"].get("spsa"):
const rawContentUrlBase = rawOfficialMaster;
const apiUrlBase = apiOfficialMaster;
% if run["args"].get("official_master_sha"):
const diffBase = baseOfficialMaster;
% else: # old tests before this field
const diffBase = "master";
dots = 3; // fall back to the three dot diff request as the diff will be rebased
% endif
% else:
const rawContentUrlBase = rawContentUrlNew;
const apiUrlBase = apiUrlNew;
const diffBase = "${run["args"]["resolved_base"][:10]}";
% endif

// Check if the diff is already in sessionStorage and use it if it is
let text = sessionStorage.getItem("${run['_id']}");
let count = sessionStorage.getItem("${run['_id']}" + "_lines");
if (!text) {
count = count == "null" ? 0 : count;

if (diffBase !== "master") // not SPSA
diffs = await fetchDiffTwoDots(rawContentUrl, apiUrl, diffBase, diffNew);
else
if (!text) {
if (dots === 2)
diffs = await fetchDiffTwoDots(rawContentUrlNew, rawContentUrlBase, apiUrlNew, apiUrlBase, diffNew, diffBase);
else if (dots === 3)
diffs = await fetchDiffThreeDots(diffApiUrl);
text = diffs.text;
count = diffs.count;

if (diffs && diffs.text) {
text = diffs.text;
count = diffs.count;
}
}
if (text) {
showDiff(text, count);
return fetchComments(diffApiUrl);

if (!text) {
text = "No diff available";
}
const diffContents = document.getElementById("diff-contents");
const diffText = diffContents.querySelector("code");
addDiff(diffText, text);
showDiff(diffContents, diffText, count);
hljs.highlightElement(diffText);
const toggleBtn = document.getElementById("diff-toggle");
toggleBtn.addEventListener("click", function () {
diffContents.style.display =
diffContents.style.display === "none" ? "" : "none";
if (copyDiffBtn)
copyDiffBtn.style.display =
copyDiffBtn.style.display === "none" ? "" : "none";
if (toggleBtn.textContent === "Hide") toggleBtn.textContent = "Show";
else toggleBtn.textContent = "Hide";
});
document.getElementById("diff-section").style.display = "";
// Try to save the diff in sessionStorage
// It can throw an exception if there is not enough space
try {
sessionStorage.setItem("${run['_id']}", text);
sessionStorage.setItem("${run['_id']}" + "_lines", count);
} catch (e) {
console.warn("Could not save diff in sessionStorage");
}

% if run["args"]["base_tag"] == "master":
if (baseOfficialMaster) {
let text = sessionStorage.getItem("${run['_id']}_master_vs_official_master");
if (!text) {
const masterVsOfficialMaster =
await fetchDiffTwoDots(rawContentUrlBase, rawOfficialMaster, apiUrlBase, apiOfficialMaster, diffBase, baseOfficialMaster);
text = masterVsOfficialMaster.text;
}

if (text) {
document.getElementById("master_vs_official_master").style.display = "";
const diffContents = document.getElementById("diff-contents");
const diffText = diffContents.querySelector("code");
document.getElementById("master_vs_official_master").addEventListener("click", (e) => {
e.currentTarget.classList.toggle("active");
if (e.currentTarget.classList.contains("active")) {
e.currentTarget.querySelector("span").textContent = "base vs master";
addDiff(diffText, text);
hljs.highlightElement(diffText);
}
else {
e.currentTarget.querySelector("span").textContent = "master vs base";
// Check if the diff is already in sessionStorage and use it if it is
const originalDiffText = sessionStorage.getItem("${run['_id']}");
const originalDiffCount = sessionStorage.getItem("${run['_id']}" + "_lines");
addDiff(diffText, originalDiffText);
showDiff(diffContents, diffText, originalDiffCount);
hljs.highlightElement(diffText);
}
});

// Try to save the diff in sessionStorage
// It can throw an exception if there is not enough space
try {
sessionStorage.setItem("${run['_id']}_master_vs_official_master", text);
} catch (e) {
console.warn("Could not save diff in sessionStorage");
}
}
}
% endif

return fetchComments(diffApiUrl);
}

const diffPromise = handleDiff();
Expand Down
23 changes: 23 additions & 0 deletions server/fishtest/views.py
Original file line number Diff line number Diff line change
Expand Up @@ -815,6 +815,18 @@ def validate_form(request):
"tests_repo": request.POST["tests-repo"],
"info": request.POST["run-info"],
}
try:
data["master_sha"] = get_master_sha(
data["tests_repo"].replace(
"https://github.com", "https://api.github.com/repos"
)
)
data["official_master_sha"] = get_master_sha(
"https://api.github.com/repos/official-stockfish/Stockfish"
)
except Exception as e:
raise Exception("Error occurred while fetching master commit signatures") from e

odds = request.POST.get("odds", "off") # off checkboxes are not posted
if odds == "off":
data["new_tc"] = data["tc"]
Expand Down Expand Up @@ -1077,6 +1089,17 @@ def new_run_message(request, run):
return ret


def get_master_sha(repo_url):
try:
repo_url += "/commits/master"
response = requests.get(repo_url).json()
if "commit" not in response:
raise Exception("Cannot find branch in repository")
return response["sha"]
except Exception as e:
raise Exception("Unable to access repository") from e


@view_config(route_name="tests_run", renderer="tests_run.mak", require_csrf=True)
def tests_run(request):
user_id = ensure_logged_in(request)
Expand Down
2 changes: 2 additions & 0 deletions server/tests/test_api.py
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,8 @@ def new_run(self, add_tasks=0):
info="The ultimate patch",
resolved_base="347d613b0e2c47f90cbf1c5a5affe97303f1ac3d",
resolved_new="347d613b0e2c47f90cbf1c5a5affe97303f1ac3d",
master_sha="347d613b0e2c47f90cbf1c5a5affe97303f1ac3d",
official_master_sha="347d613b0e2c47f90cbf1c5a5affe97303f1ac3d",
msg_base="Bad stuff",
msg_new="Super stuff",
base_signature="123456",
Expand Down
4 changes: 4 additions & 0 deletions server/tests/test_rundb.py
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,8 @@ def test_10_create_run(self):
info="The ultimate patch",
resolved_base="347d613b0e2c47f90cbf1c5a5affe97303f1ac3d",
resolved_new="347d613b0e2c47f90cbf1c5a5affe97303f1ac3d",
master_sha="347d613b0e2c47f90cbf1c5a5affe97303f1ac3d",
official_master_sha="347d613b0e2c47f90cbf1c5a5affe97303f1ac3d",
msg_base="Bad stuff",
msg_new="Super stuff",
base_signature="123456",
Expand Down Expand Up @@ -109,6 +111,8 @@ def test_10_create_run(self):
info="The ultimate patch",
resolved_base="347d613b0e2c47f90cbf1c5a5affe97303f1ac3d",
resolved_new="347d613b0e2c47f90cbf1c5a5affe97303f1ac3d",
master_sha="347d613b0e2c47f90cbf1c5a5affe97303f1ac3d",
official_master_sha="347d613b0e2c47f90cbf1c5a5affe97303f1ac3d",
msg_base="Bad stuff",
msg_new="Super stuff",
base_signature="123456",
Expand Down

0 comments on commit b5c9729

Please sign in to comment.